feat(webcodecs): skip decoding/rendering frames when decoder/renderer can't keep up #827
Conversation
📝 WalkthroughWalkthroughRenamed PTY/Adb types; migrated codec decoders to TransformStream-based codecs; promoted PauseController/PerformanceCounter to public stream primitives; added streaming pipeline components (VideoDecoderStream, TimestampTransforms, RendererController, RedrawController) and many renderer/utility API changes across scrcpy and adb modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PauseController
participant TimestampTransforms
participant VideoDecoderStream
participant RendererController
participant Renderer
Client->>PauseController: write packets (config/data)
PauseController-->>TimestampTransforms: output packets (adds skipRendering)
TimestampTransforms->>VideoDecoderStream: forward timestamped packets/config
VideoDecoderStream->>RendererController: emit decoded VideoFrame
RendererController->>Renderer: deliver frames via writable/readable and update counters
sequenceDiagram
participant App as WebCodecsVideoDecoder
participant Pause as PauseController
participant Codec as CodecTransformStream
participant VD as VideoDecoderStream
participant RC as RendererController
App->>Pause: pipe input stream
Pause->>Codec: pipe transformed packets
Codec->>VD: emit EncodedVideoChunk or Config
VD->>RC: output VideoFrame
RC->>App: expose metrics, snapshot, readable/writable
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2026-01-12T06:58:27.724ZApplied to files:
📚 Learning: 2025-08-24T03:50:43.091ZApplied to files:
📚 Learning: 2026-01-12T06:58:19.519ZApplied to files:
🧬 Code graph analysis (2)libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (4)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (20)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI Agents
In @libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts:
- Line 211: The dispose() method can throw if called before the microtask that
initializes this.#controller completes; guard the call by checking that
this.#controller is defined before invoking terminate(). Update dispose() to do
something like: if (this.#controller) this.#controller.terminate(); (or
null-check and no-op) so calling dispose() immediately after construction is
safe without relying on initialization timing for the controller.
- Around line 142-152: The code in resume() can access frames[frames.length - 1]
when this.#pendingFrames is empty causing a spread of undefined; add a guard at
the start (e.g., const frames = this.#pendingFrames.slice(); if (frames.length
=== 0) return;) to bail out early, then proceed with the existing loop and final
enqueue; reference this.#pendingFrames, resume(), and this.#controller.enqueue
when making the change.
- Around line 50-55: The constructor defers setting private field #controller
via "await Promise.resolve()", which can leave #controller undefined if
dispose(), resume(), or #resumeInternal() are invoked synchronously after
construction; fix by ensuring #controller is assigned synchronously in the
constructor (remove the microtask await) or, alternatively, introduce a ready
promise (e.g., this.controllerReady) that resolves when controller is set and
await that promise at the start of dispose(), resume(), and #resumeInternal() to
guard access; update references in constructor, dispose(), resume(), and
#resumeInternal() accordingly.
In @libraries/scrcpy-decoder-webcodecs/src/video/time.ts:
- Around line 21-28: The increasingNow function violates strict monotonicity by
calling nextUp(now) when now <= prevValue; change the branch so it calls
nextUp(prevValue) instead, assign that result to prevValue, and return it;
ensure you still call performance.now() to get now, compare with prevValue, and
only call nextUp(prevValue) when now <= prevValue so the returned value is
strictly greater than the prior prevValue.
In @libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts:
- Around line 71-84: The code calls this.#decoder.configure(this.#config!) after
resetting the decoder but uses a non-null assertion for the optional field
this.#config; add an explicit runtime guard before calling configure (check if
this.#config is falsy and throw a clear Error such as "Decoder not configured")
so decoding paths like the keyframe handler in video-decoder-stream.ts validate
configuration instead of relying on the protocol; keep the throw synchronous so
callers see a clear error and only call this.#decoder.configure when
this.#config is present.
🧹 Nitpick comments (1)
libraries/scrcpy-decoder-tinyh264/src/decoder.ts (1)
114-139: Consider logging or handling pipeline errors instead of silently swallowing them.The
.catch(noop)on line 138 silently discards any errors from the streaming pipeline. While thepipeTopromise is intentionally not awaited (fire-and-forget), silently swallowing errors could make debugging difficult if the pipeline fails unexpectedly.Based on learnings, configuration failures should propagate up rather than being recovered locally. Consider at minimum logging the error for diagnostic purposes.
🔎 Proposed improvement
void this.#pause.readable .pipeTo( new WritableStream({ write: async (packet) => { // ... existing logic }, }), ) - .catch(noop); + .catch((error) => { + // Pipeline errors are expected on dispose, but log unexpected errors + if (error && !(error instanceof TypeError && error.message.includes("terminated"))) { + console.error("TinyH264Decoder pipeline error:", error); + } + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
libraries/adb/src/commands/subprocess/none/pty.tslibraries/adb/src/commands/subprocess/none/service.tslibraries/adb/src/commands/subprocess/pty.tslibraries/adb/src/commands/subprocess/shell/pty.tslibraries/adb/src/commands/subprocess/shell/service.tslibraries/scrcpy-decoder-tinyh264/src/decoder.tslibraries/scrcpy-decoder-tinyh264/src/types.tslibraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-tinyh264/src/utils/performance.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/av1.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h264.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h265.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/type.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/time.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.tslibraries/struct/src/utils.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-03-30T19:57:44.305Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 739
File: apps/cli/src/index.ts:163-167
Timestamp: 2025-03-30T19:57:44.305Z
Learning: The none protocol in ADB doesn't pass exit codes (always returns 0), unlike the shell protocol. The API is designed to make this distinction clear by using `exited` instead of `exit` with a code parameter for none protocol.
Applied to files:
libraries/adb/src/commands/subprocess/none/pty.tslibraries/adb/src/commands/subprocess/pty.tslibraries/adb/src/commands/subprocess/none/service.ts
📚 Learning: 2025-08-23T14:04:38.056Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 786
File: libraries/adb/src/commands/sync/sync.ts:154-157
Timestamp: 2025-08-23T14:04:38.056Z
Learning: Both ADB none protocol (`exec:`) and shell protocol (`shell:`) are executed by shell using `sh -c ...` on the Android side and require proper argument escaping. The two protocols only differ in how standard input/output are encoded for transmission, not in their execution model. The `escapeArg()` function should be used for both protocols to prevent command injection when paths contain special characters.
Applied to files:
libraries/adb/src/commands/subprocess/shell/service.tslibraries/adb/src/commands/subprocess/shell/pty.tslibraries/adb/src/commands/subprocess/none/service.ts
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h264.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h265.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-tinyh264/src/decoder.ts
📚 Learning: 2025-08-14T11:06:21.851Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 780
File: libraries/adb-credential-web/src/storage/index.ts:1-5
Timestamp: 2025-08-14T11:06:21.851Z
Learning: The yume-chan/adb-credential-web package doesn't use "exports" in package.json for better compatibility. Instead, they re-export everything from the main entrypoint rather than using subpath exports.
Applied to files:
libraries/adb/src/commands/subprocess/none/service.ts
🧬 Code graph analysis (13)
libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPerformanceCounter(15-36)
libraries/adb/src/commands/subprocess/none/pty.ts (1)
libraries/adb/src/commands/subprocess/pty.ts (1)
AdbPty(8-15)
libraries/adb/src/commands/subprocess/shell/service.ts (1)
libraries/adb/src/commands/subprocess/shell/pty.ts (1)
AdbShellProtocolPty(20-112)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (4)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
Input(13-15)Output(22-22)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPauseController(38-63)libraries/adb-scrcpy/src/client.ts (1)
controller(285-287)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (2)
libraries/adb-scrcpy/src/video.ts (3)
data(76-79)data(81-84)data(86-100)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
CodecDecoder(7-10)Config(17-20)
libraries/adb/src/commands/subprocess/shell/pty.ts (1)
libraries/adb/src/commands/subprocess/pty.ts (1)
AdbPty(8-15)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (4)
CodecDecoder(7-10)Input(13-15)Output(22-22)Config(17-20)libraries/stream-extra/src/concat.ts (1)
concatUint8Arrays(120-136)
libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (2)
Input(221-221)Output(222-224)
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (2)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (1)
libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
CodecDecoder(7-10)Config(17-20)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
CodecDecoder(7-10)Input(13-15)Output(22-22)libraries/media-codec/src/av1.ts (1)
Av1(197-762)
libraries/adb/src/commands/subprocess/none/service.ts (1)
libraries/adb/src/commands/subprocess/none/pty.ts (1)
AdbNoneProtocolPty(12-45)
libraries/scrcpy-decoder-tinyh264/src/decoder.ts (2)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
PauseController(12-218)libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
PerformanceCounter(3-64)
🪛 Biome (2.1.2)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
[error] 296-296: Unexpected constant condition.
(lint/correctness/noConstantCondition)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: diffray code review
- GitHub Check: build (20.x)
🔇 Additional comments (40)
libraries/struct/src/utils.ts (2)
11-14: LGTM! Adding standard TextEncoder method.The
encodeIntomethod is part of the standard TextEncoder API and is more efficient thanencode()for performance-critical paths where you can reuse buffers. The signature correctly matches the specification.
32-32: LGTM! Exposing shared encoder instance.Making
SharedEncoderpublic is a reasonable optimization that allows consuming code to reuse the singleton instance. SinceTextEncoderis stateless, sharing the instance is safe.libraries/adb/src/commands/subprocess/shell/service.ts (3)
5-5: LGTM! Import properly updated.The import correctly references the renamed
AdbShellProtocolPtyclass.
42-42: LGTM! Return type properly updated.The return type correctly reflects the renamed class.
62-62: LGTM! Instantiation properly updated.The instantiation correctly uses the renamed
AdbShellProtocolPtyclass.libraries/adb/src/commands/subprocess/none/pty.ts (2)
10-10: LGTM! Import properly updated.The import correctly references the renamed
AdbPtyinterface.
12-45: LGTM! Class rename with no behavioral changes.The class rename from
AdbNoneProtocolPtyProcesstoAdbNoneProtocolPtyis cleaner and more consistent. The implementation remains unchanged, maintaining all functionality.libraries/adb/src/commands/subprocess/shell/pty.ts (2)
16-16: LGTM! Import properly updated.The import correctly references the renamed
AdbPtyinterface.
20-112: LGTM! Class rename with no behavioral changes.The class rename from
AdbShellProtocolPtyProcesstoAdbShellProtocolPtyis cleaner and more consistent. The implementation remains unchanged, maintaining all functionality including exit code handling, stdout streaming, and terminal control.libraries/adb/src/commands/subprocess/none/service.ts (3)
4-4: LGTM! Import properly updated.The import correctly references the renamed
AdbNoneProtocolPtyclass.
45-45: LGTM! Return type properly updated.The return type correctly reflects the renamed class.
55-55: LGTM! Instantiation properly updated.The instantiation correctly uses the renamed
AdbNoneProtocolPtyclass.libraries/adb/src/commands/subprocess/pty.ts (1)
8-15: LGTM! Clean interface rename.The interface rename from
AdbPtyProcesstoAdbPtyis cleaner and more concise. All references across the codebase have been properly updated, including the implementing classes (AdbNoneProtocolPtyandAdbShellProtocolPty) and all imports. The interface contract remains unchanged with no breaking modifications.libraries/scrcpy-decoder-tinyh264/src/types.ts (2)
15-36: LGTM!The renamed properties (
framesRendered,framesPresented,framesSkippedRendering) provide clearer semantics, and the documentation accurately explains the distinction between rendered and presented frames, including the Chrome bug caveat.
52-52: Good flexibility withMaybePromiseLike.Changing the return type from
Promise<undefined>toMaybePromiseLike<undefined>allows implementations to return synchronously when no async work is needed, which is a good performance-conscious design choice.libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
3-17: LGTM!The class correctly implements
ScrcpyVideoDecoderPerformanceCounterwith the renamed getters. Keeping the internal field names (#framesDrawn,#framesSkipped) while renaming only the public API is a reasonable choice that minimizes churn.libraries/scrcpy-decoder-tinyh264/src/decoder.ts (3)
44-51: LGTM!Clean delegation to
PauseControllerfor both thepausedstate andwritablestream. This aligns well with the stream-based architecture.
64-90: LGTM!The performance counter getters are properly delegated with clear documentation that matches the interface definitions.
223-225: Verify return type matches interface.The
resume()method now returnsundefinedsynchronously, while the interfaceScrcpyVideoDecoderPauseController.resume()specifiesMaybePromiseLike<undefined>. This is compatible sinceundefinedsatisfiesMaybePromiseLike<undefined>, but ensure this is intentional given the previous async behavior.libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
12-15: Good design: TransformStream-based pause controller with typed namespace.The
PauseControllerextendingTransformStreamis a clean streaming-based design. The namespace exports provide well-typedInputandOutputtypes that make the data flow explicit.Also applies to: 220-225
libraries/scrcpy-decoder-webcodecs/src/video/time.ts (1)
3-17: LGTM!The
nextUpimplementation correctly handles special cases and uses bit manipulation to compute the next representable float64 value.libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (3)
48-49: Clarify the async Promise.resolve pattern.The
await Promise.resolve()pattern is unusual. If this is intentional to defer initialization to the next microtask, consider adding a comment explaining why.
64-69: LGTM!Configuration handling is correct - stores the config for potential decoder resets and immediately configures the decoder.
86-89: LGTM!Flush handling correctly waits for pending decodes before closing the decoder.
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (5)
195-259: LGTM! Streaming pipeline is well-structured.The pipeline composition correctly chains pause control, timestamp annotation, codec decoding, configuration inspection, frame decoding, and rendering. The use of
increasingNow()for timestamps enables both decode time tracking and frame skipping logic.Note: Line 259 uses
catch(noop)which silently suppresses errors. This appears intentional for cleanup scenarios, but ensure critical errors are logged elsewhere if needed.
262-299: LGTM! The do-while loop is correct.The
while (true)at line 296 is used correctly in a do-while loop with an explicit break condition at line 294. This pattern ensures all queued frames are rendered in sequence, minimizing latency. The static analysis warning is a false positive.The frame queuing strategy (lines 264-271) correctly handles backpressure by dropping intermediate frames when the renderer can't keep up.
347-357: LGTM! Options interface is appropriately simplified.The interface correctly exposes only the relevant decoder configuration options while requiring the essential codec and renderer parameters.
320-326: LGTM!Pause/resume methods correctly delegate to the PauseController.
179-193: LGTM!Codec selection logic is clear and handles unsupported codecs appropriately.
libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (1)
7-15: LGTM! Configuration method aligns with streaming architecture.The
configuremethod correctly returns aCodecDecoder.Configobject instead of performing side effects, which aligns with the new stream-based decoder architecture.libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (1)
7-18: LGTM! Configuration method matches H264 pattern.The
configuremethod correctly returns aCodecDecoder.Configobject. The comment about Microsoft Edge's explicit size requirement (lines 12-14) provides valuable context for the implementation.libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (3)
5-8: Clean stream-based refactor with proper Annex B handling.The class now properly extends
TransformStreamwith the appropriate input/output types. The design cleanly separates configuration from data processing.
11-42: Transform logic is correct for H.264/H.265 Annex B format.The implementation correctly:
- Stores configuration and emits the parsed config on configuration packets
- Concatenates configuration with the first frame data (per WebCodecs AVC/HEVC Annex B requirements)
- Clears the stored config after use to avoid re-concatenation
- Treats
undefinedkeyframe askey(necessary for proper decoding)The reference to the W3C WebCodecs spec in the comment is helpful for maintainability.
44-44: Abstract method signature aligns with the streaming design.The
configuremethod now returnsCodecDecoder.Configinstead ofvoid, enabling the transformer to emit configuration downstream. This is consistent with theOutputtype definition.libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (3)
6-9: Consistent streaming pattern with H26x decoder.The class properly extends
TransformStreamwith the same input/output types, maintaining consistency with the other codec implementations.
18-30: Config emission depends on sequence header presence in each packet.The implementation correctly parses for sequence headers on every data packet and emits config when found. For AV1, sequence headers can appear in any keyframe, so this on-demand config emission is appropriate.
One consideration: if the downstream consumer requires config before processing chunks, ensure it handles the case where the first few packets might not contain a sequence header. Based on the
Outputtype beingConfig | EncodedVideoChunk, downstream should be prepared to handle either.
32-39: EncodedVideoChunk creation is correct.The keyframe handling (
packet.keyframe === false ? "delta" : "key") is consistent with the H26x implementation and the comment explains the rationale clearly.libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
7-10: Clean type alias design for streaming decoders.The
CodecDecodertype alias cleanly defines the streaming contract. Using a namespace for associated types (Input,Config,Output) is a good TypeScript pattern for grouping related types.
25-27: Simplified constructor signature.The parameterless constructor is cleaner and aligns with the new streaming architecture where decoders are self-contained TransformStreams.
12-22: Well-structured type definitions.The types are well-designed:
Inputcorrectly unions configuration and timestamped data packetsConfigextendsVideoDecoderConfigwith required dimensions (needed for sizing)Outputunion enables emitting both config and encoded chunksThe tsconfig includes
"DOM"lib, which provides the WebCodecs API types (VideoDecoderConfigandEncodedVideoChunk) as globals, so they are properly available.
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
Outdated
Show resolved
Hide resolved
diffray
bot
added
diffray-review-failed
diffray
bot
added
diffray-review-started
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
120-167: Non-null assertions unsafe due to constructor race condition.Lines 142 and 157 use non-null assertions (
this.#controller!) which are unsafe given the asynchronous controller initialization in the constructor. These assertions will throw if#resumeInternalis called before the microtask completes.The iteration logic using
entries()correctly handles empty arrays and properly assigns theskipRenderingflag.
🤖 Fix all issues with AI Agents
In @libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts:
- Around line 52-63: The constructor's start handler defers assigning the
private field #controller via await Promise.resolve(), causing a race where
dispose(), resume(), or other methods may access an undefined #controller;
remove the unnecessary await Promise.resolve() so the assignment
this.#controller = controller happens synchronously in the start callback, and
also add a defensive guard (e.g., if (!this.#controller) return or no-op) in
dispose() and resume() to avoid non-null assertion failures when they are called
before controller is set.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.vscode/settings.jsonlibraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/time.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
🧬 Code graph analysis (2)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
Input(13-15)Output(22-22)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPauseController(38-63)
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (1)
frame(262-299)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (20.x)
- GitHub Check: diffray code review
🔇 Additional comments (9)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (4)
64-103: Transform logic correctly handles pause/resume flow.The transform callback properly manages paused state by queuing configuration and data packets, and correctly awaits any in-progress resume operation before enqueueing new packets. The use of the
controllerparameter (notthis.#controller) avoids race condition issues.
169-171: Resume signature correctly implements interface.The return type change from
MaybePromiseLike<undefined>toundefinedis valid, asundefinedsatisfies the interface requirement forMaybePromiseLike<undefined>.
213-221: Good defensive programming with optional chaining.Line 220 correctly uses optional chaining (
this.#controller?.terminate()) to handle the case where#controllermight be undefined. This resolves the past crash issue, though the root cause (constructor race condition) should still be addressed.
229-234: Clean public API definition.The namespace exports provide a clear and well-structured public API for the PauseController, with
InputandOutputtypes that accurately reflect the streaming transformation.libraries/scrcpy-decoder-webcodecs/src/video/time.ts (2)
1-17: Correct IEEE-754 implementation for next representable value.The
nextUpfunction correctly implements IEEE-754 bit manipulation to compute the next representable double-precision value. Special cases (NaN, Infinity, zero) are properly handled, and the bit arithmetic correctly increments for positive values and decrements for negative values.
21-28: Monotonicity correctly implemented.The function now correctly calls
nextUp(prevValue)(line 24) instead ofnextUp(now)whennow <= prevValue, ensuring strict monotonicity as required. The past review issue has been resolved.libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (3)
46-63: Decoder initialization correctly structured.The constructor properly initializes the VideoDecoder within the TransformStream's
startcallback, ensuring the decoder is ready before any transform operations. The output callback correctly increments the frames counter and enqueues frames, while the error callback propagates errors appropriately.
64-73: Config handling with appropriate guards.Lines 65-68 correctly save the configuration and configure the decoder. The guard at lines 71-73 ensures the decoder is configured before processing chunks, with a clear error message. This addresses the protocol requirement that configuration must precede data chunks.
90-93: Proper flush and cleanup logic.The flush callback correctly awaits pending decode operations before closing the decoder, ensuring clean shutdown of the video pipeline.
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
Outdated
Show resolved
Hide resolved
diffray
bot
added
diffray-review-failed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
50-98: Excellent fix for the race condition!The closure pattern elegantly solves the previous race condition by capturing
controllersynchronously duringsuper()construction, then assigning it to#controllerimmediately after. This ensures#controlleris always initialized before any methods can access it.The transform logic correctly handles:
- Buffering configuration and frames when paused
- Clearing pending frames on new keyframes
- Blocking during resume with
await this.#resuming- Enqueuing with appropriate
skipRenderingflagsOptional: Add clarifying comment
Consider adding a brief comment explaining the closure pattern for future maintainers:
constructor() { + // Capture controller via closure to ensure synchronous initialization let controller!: TransformStreamDefaultController<PauseController.Output>;libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (1)
91-94: Consider adding cleanup for abnormal stream termination.While the flush callback properly closes the decoder, this only runs on normal stream completion. If the stream is cancelled or errors before flush completes, the decoder may remain open (though it will eventually be garbage collected).
For cleaner resource management, consider adding an abort signal or catch handler to explicitly close the decoder on abnormal termination.
💡 Optional enhancement for robust cleanup
You could capture abort/cancel by listening to the controller's abort signal or by adding error handling in the transform. However, given the current architecture and the fact that the decoder will be GC'd eventually, this is more of a nice-to-have than a must-fix.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
🧬 Code graph analysis (2)
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (1)
frame(262-299)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (4)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
Input(13-15)Output(22-22)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPauseController(38-63)libraries/adb-scrcpy/src/client.ts (1)
controller(285-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (10)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (3)
145-156: Resume logic correctly handles all cases.The iteration using
.entries()safely handles empty#pendingFramesarrays (the loop simply won't execute), and theskipRenderinglogic correctly marks all frames except the last one. This resolves the previous critical issue about accessing empty arrays.The comment at lines 145-147 accurately explains the synchronization guarantee.
164-165: Disposal and return type are correct.
- The
resume()return typeundefinedcorrectly satisfies theMaybePromiseLike<undefined>interface requirement.dispose()safely callsterminate()because#controlleris always initialized via the constructor's closure pattern, resolving the previous crash concern.Also applies to: 208-221
224-229: Clean type exports using namespace pattern.The namespace pattern cleanly associates
InputandOutputtypes with thePauseControllerclass, making the API surface clear and type-safe.libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (7)
1-7: LGTM! Clean stream-based architecture.The class properly extends
TransformStreamwith appropriate input/output types. The union input typeVideoDecoderConfig | EncodedVideoChunkelegantly handles both configuration and data packets in a single stream, aligning well with the PR's goal of converting to stream-based decoders.
8-44: Well-designed public API surface.The exposed metrics (
decodeQueueSize,framesDecoded,framesSkipped) andonDequeueevent provide excellent observability into decoder behavior, which is essential for monitoring performance and latency in streaming video applications.
46-64: Proper decoder initialization and event wiring.The initialization pattern correctly creates the
VideoDecoderin thestartcallback and wires up output/error handlers. The error propagation on line 58 ensures configuration failures and decode errors bubble up the pipeline as expected.
65-70: Config handling looks correct.The type guard on line 66 appropriately distinguishes
VideoDecoderConfigfromEncodedVideoChunk. Storing the config before callingconfigure()is essential for the keyframe reset logic later in the transform.
72-74: Excellent defensive coding.The explicit state check with a clear error message ensures that encoded chunks are only processed after configuration. This validation makes the non-null assertion on line 86 safe, as the decoder can only be configured through line 68, which always sets
#configfirst.
76-87: Smart latency management with justified non-null assertion.The keyframe-based frame dropping strategy effectively caps latency when the decoder can't keep up. The non-null assertion on line 86 is safe because the state check on line 72 guarantees the decoder was previously configured via line 68, which always sets
#configbefore callingconfigure().
89-89: LGTM!The decode call is straightforward and safe at this point, with proper error handling wired through the decoder's error callback.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts:
- Around line 129-141: The snapshot() method lacks an explicit return type;
update its signature to declare the correct async return type (Promise<Blob |
undefined>) so callers and the compiler know it returns either a Blob or
undefined, keeping the existing logic that borrows a VideoFrameCapturerPool,
calls capturer.capture(frame), and returns that result; ensure the method
signature on the snapshot() function is changed accordingly.
- Around line 102-106: The close handler currently only closes
this.#readableController and disposes this.#counter, risking leaked VideoFrame
objects; update the close function where the ReadableStream is created (the
close: () => { ... } block) to also check and close any pending VideoFrame
references by calling close() on this.#captureFrame and this.#nextFrame if they
are non-null and then nulling them out before disposing the counter and closing
the controller so all frames are released at shutdown.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts:
- Around line 1-5: The interface uses the nonstandard property name `writeable`;
change `VideoFrameRenderer.writeable` to `writable` to match the standard stream
API and the `RendererController` usage in `flow-control.ts`, then update all
implementations and references to `VideoFrameRenderer` (including any
classes/functions that consume or construct that interface) to use `writable` so
names are consistent across the codebase.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts:
- Around line 14-60: createProgram may receive null from gl.createProgram() if
the WebGL context is lost; add an immediate null check after const program =
gl.createProgram() in createProgram and handle it before calling
gl.attachShader/linkProgram. Specifically, if program is null, delete the
vertexShader and fragmentShader (to avoid leaks) and throw a clear Error (e.g.,
"Failed to create program: WebGL context lost") so subsequent
gl.attachShader/gl.linkProgram calls never run on a null program.
In @libraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts:
- Around line 13-16: The explicit cast "(this.#canvas as HTMLCanvasElement)" is
unnecessary because both OffscreenCanvas and HTMLCanvasElement support
getContext("bitmaprenderer"); remove the cast and call
this.#canvas.getContext("bitmaprenderer", { alpha: false })! to preserve type
safety; if TypeScript still complains about the union type, either add a short
comment explaining why a cast is required or narrow the type first (e.g., check
instanceof HTMLCanvasElement or assert via a single localized cast) while
keeping the comment explaining the reason.
🧹 Nitpick comments (5)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (1)
18-30: Consider caching to avoid redundant sequence header parsing.The code instantiates a new
Av1parser and searches for sequence headers on every packet. Since AV1 sequence headers typically appear only once at stream start, this creates unnecessary overhead for the majority of packets that won't contain sequence headers.Consider tracking whether a configuration has already been emitted and skipping the parsing for subsequent packets, unless dynamic resolution changes are expected.
♻️ Potential optimization approach
+ private configSent = false; + constructor() { super({ transform: (packet, controller) => { if (packet.type === "configuration") { // AV1 decoder doesn't need configuration packets return; } - const parser = new Av1(packet.data); - const sequenceHeader = parser.searchSequenceHeaderObu(); - - if (sequenceHeader) { + if (!this.configSent) { + const parser = new Av1(packet.data); + const sequenceHeader = parser.searchSequenceHeaderObu(); + + if (sequenceHeader) { - const width = sequenceHeader.max_frame_width_minus_1 + 1; - const height = sequenceHeader.max_frame_height_minus_1 + 1; - - controller.enqueue({ - codec: Av1.toCodecString(sequenceHeader), - codedWidth: width, - codedHeight: height, - }); + const width = sequenceHeader.max_frame_width_minus_1 + 1; + const height = sequenceHeader.max_frame_height_minus_1 + 1; + + controller.enqueue({ + codec: Av1.toCodecString(sequenceHeader), + codedWidth: width, + codedHeight: height, + }); + this.configSent = true; + } } controller.enqueue(Note: If the stream supports dynamic resolution changes, you may want to check for sequence headers periodically rather than only once.
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (1)
13-15: Consider documenting the pool capacity choice.The pool capacity of
4appears to be a magic number. Consider adding a comment explaining the rationale for this size (e.g., expected concurrent snapshot operations, memory constraints, etc.).📝 Suggested documentation improvement
+// Pool size of 4 allows up to 4 concurrent snapshot operations +// while keeping memory usage bounded const VideoFrameCapturerPool = /* #__PURE__ */ new Pool(() => new VideoFrameCapturer(), 4);libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (1)
11-14: Consider aligning bitmaprenderer context configurations across renderers.The
alpha: trueconfiguration in bitmap.ts follows MDN best practices to avoid the performance cost ofalpha: false. However, snapshot.ts (line 15) still usesalpha: falsefor its ImageBitmapRenderingContext in the VideoFrameCapturer class. While the use cases differ (rendering vs. capturing), inconsistent configurations can create confusion. Either align both to usealpha: truefollowing the MDN guidance, or document why snapshot.ts requiresalpha: false.libraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.ts (1)
24-26: Consider renaming getter to match standard spelling.The getter is named
writeablebut exposeswritablefrom the generator. The standard spelling is "writable" (as used in the Web Streams API and line 25). Consider renaming for consistency.📝 Suggested rename
- get writeable() { + get writable() { return this.#generator.writable; }libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
19-35: Frame lifecycle looks correct, but consider getter naming.The stream implementation properly manages frame lifecycle by closing the previous frame before saving the new one. The close/abort handlers correctly call
dispose().However, the getter is named
writeable(non-standard spelling) instead ofwritable(standard Web Streams API spelling). This inconsistency also appears ininsertable-stream.ts.📝 Suggested rename for consistency
- get writeable() { + get writable() { return this.#writable; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
libraries/scrcpy-decoder-tinyh264/src/decoder.tslibraries/scrcpy-decoder-tinyh264/src/utils/performance.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/av1.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/index.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/utils.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.tslibraries/scrcpy-decoder-webcodecs/src/video/render/index.tslibraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.tslibraries/scrcpy-decoder-webcodecs/src/video/render/type.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.tslibraries/scrcpy-decoder-webcodecs/src/video/snapshot.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts
💤 Files with no reviewable changes (2)
- libraries/scrcpy-decoder-webcodecs/src/video/codec/utils.ts
- libraries/scrcpy-decoder-webcodecs/src/video/codec/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-tinyh264/src/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
🧬 Code graph analysis (8)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
libraries/stream-extra/src/consumable.ts (1)
WritableStream(71-71)
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (4)
libraries/scrcpy-decoder-webcodecs/src/video/pool.ts (1)
Pool(1-37)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPerformanceCounter(15-36)libraries/stream-extra/src/push-readable.ts (2)
PushReadableStreamController(6-14)PushReadableStream(41-307)libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
PerformanceCounter(3-64)
libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (2)
libraries/scrcpy-decoder-tinyh264/src/decoder.ts (1)
canvas(40-42)libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
canvas(8-10)
libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (2)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
CodecDecoder(7-10)Input(13-15)Output(22-22)libraries/media-codec/src/av1.ts (1)
Av1(197-762)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (4)
CodecDecoder(7-10)Input(13-15)Output(22-22)Config(17-20)libraries/stream-extra/src/concat.ts (1)
concatUint8Arrays(120-136)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (2)
glCreateContext(11-59)glLoseContext(61-67)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (2)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
Options(205-212)libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (1)
createCanvas(1-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (38)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (3)
1-4: LGTM!The imports are clean and correctly reference the necessary dependencies for the stream-based codec implementation.
6-9: LGTM!The class declaration correctly extends
TransformStreamand implementsCodecDecoder, aligning with the PR's objective to convert decoders to a stream-based architecture.
32-39: LGTM!The
EncodedVideoChunkgeneration correctly handles the keyframe fallback—treatingundefinedas"key"is a safe default that ensures decodability. The packet data and timestamp are properly forwarded to the chunk.libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (4)
1-1: LGTM!The new imports are properly used:
concatUint8Arraysfor merging configuration with frame data (line 26), andTransformStreamfor the stream-based decoder architecture (line 6).
5-8: LGTM!The refactor to extend
TransformStream<CodecDecoder.Input, CodecDecoder.Output>correctly aligns the decoder with the stream-based architecture, replacing the previous direct VideoDecoder approach.
11-42: LGTM!The transform logic correctly implements the stream-based decoding:
- Configuration packets are stored and the
configure()result is enqueued for downstream processing- Data packets properly handle Annex B format by concatenating configuration with the first frame (lines 25-27), then clearing the config
- The keyframe type derivation (line 35) correctly treats
undefinedas"key"to ensure frames decode properlyThe absence of error handling allows configuration failures to propagate up the client pipeline, which aligns with the intended error handling strategy.
Based on learnings, configuration failures should propagate to the client pipeline for proper error handling.
44-44: All subclasses have been updated to implement the new signature. BothH264DecoderandH265Decodercorrectly overrideconfigure()with the return typeCodecDecoder.Config, and the return values are properly enqueued into the stream.libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (2)
1-44: LGTM: Well-structured stream wrapper with clean metrics API.The class structure is sound. The definite assignment assertion on line 8 is necessary because the field is assigned after the
super()call. The public getters provide a clean observability surface.
46-103: LGTM: Stream implementation correctly handles decoder lifecycle.The constructor properly implements the TransformStream pattern:
- Start handler initializes the decoder with appropriate callbacks
- Transform handler validates configuration state before decoding
- Flush handler correctly awaits pending frames before closing
- Cancel handler immediately closes on error
The implementation follows best practices for WebCodecs decoder management.
libraries/scrcpy-decoder-tinyh264/src/decoder.ts (4)
26-28: LGTM: Appropriate noop export for error handling.Exporting the noop function supports the streaming pattern used throughout the refactoring, particularly for
.catch(noop)on void promises.
44-90: LGTM: Improved metric naming and documentation.The refactoring to use
PauseControllerandPerformanceCounterimproves the public API surface. The renamed metrics (framesRendered,framesSkippedRendering) better distinguish rendering from decoding stages, and the documentation clearly explains frame presentation semantics including the Chrome bug caveat.
114-138: LGTM: Streaming pipeline correctly handles TinyH264 constraints.The refactored pipeline properly:
- Validates decoder configuration before processing data packets
- Documents the TinyH264 limitation regarding frame metadata and skipping
- Copies packet data to avoid BufferCombiner issues
- Uses the standard void/catch(noop) pattern for fire-and-forget error handling
208-208: No action required. ThePerformanceCounterclass correctly provides theincreaseFramesRendered()method at line 54 inlibraries/scrcpy-decoder-tinyh264/src/utils/performance.ts, which increments the#framesDrawncounter. The method is properly used at line 208 in the decoder.libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (5)
42-57: LGTM: Clean pause controller with timestamp-based frame skipping.The
PauseControllerintegration and#skipFramesUntilmechanism provide a robust approach to handling frame skipping during pause/resume cycles using monotonic timestamps.
74-115: LGTM: Well-documented decode time tracking rationale.The delegation to
VideoDecoderStreamis clean, and the comment at lines 104-108 clearly explains whytotalDecodeTimeis tracked at this level rather than in the stream—it includes pre-processing time and relies on timestamps set by this class.
117-147: LGTM: Comprehensive rendering metrics with clear documentation.The
RendererControllerintegration provides good observability. The documentation onframesPresented(lines 127-135) effectively explains the latency optimization and documents the Chrome nested Worker limitation.
152-176: LGTM: Clean codec selection and well-typed Options interface.The constructor properly instantiates codec-specific decoders with appropriate defaults. The
Optionsinterface cleanly extends relevantVideoDecoderConfigproperties, and the default values (hardwareAcceleration = "no-preference",optimizeForLatency = true) align well with low-latency streaming requirements.Also applies to: 269-279
177-240: No action required. Line 239 correctly usesrenderer.writeable, which matches theVideoFrameRendererinterface definition intype.ts. While the standard Web Streams API useswritable, this codebase intentionally defines and consistently useswriteableas the property name across allVideoFrameRendererimplementations (CanvasVideoFrameRenderer,InsertableStreamVideoFrameRenderer).Likely an incorrect or invalid review comment.
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (4)
78-101: LGTM! Well-designed frame queuing with proper lifecycle management.The write logic correctly handles:
- Cloning frames for snapshot capture (Line 82) to avoid ownership conflicts
- Skipping intermediate frames (Lines 88-91) to minimize latency
- Fire-and-forget drawing (Line 100) to avoid blocking the writable stream
This design aligns with the official Scrcpy client's low-latency approach.
109-127: LGTM! Proper backpressure handling with frame draining.The
#drawmethod correctly:
- Drains all queued frames in a loop (Lines 120-124)
- Respects backpressure via
awaitonenqueue(Line 122)- Updates performance metrics (Line 123)
The comments on Lines 112-117 provide excellent context for the latency-focused design choice.
71-77: LGTM! Proper abort signal wiring.The writable's
starthandler correctly propagates cancellation from the readable stream to the writable stream (Lines 74-75), ensuring proper cleanup when the consumer cancels.
38-64: LGTM! Clean delegation to PerformanceCounter.The performance counter getters correctly delegate to the internal
PerformanceCounterinstance with comprehensive JSDoc documentation that matches the interface contract.libraries/scrcpy-decoder-webcodecs/src/video/render/index.ts (1)
5-5: LGTM! Properly exports the new flow-control module.The export is consistent with the existing pattern and makes the
RendererControllerclass available as part of the public render API.libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (1)
6-7: Reference the HTML spec clarifying ImageBitmap width/height behavior.The HTML specification explicitly states that
transferFromImageBitmap()makes the context's output bitmap refer to the provided ImageBitmap, with its intrinsic size derived from the ImageBitmap itself—not the canvas dimensions. ImageBitmap width/height can differ from canvas width/height; that's the intended behavior. Update the comment to reflect this, or clarify if there's a different reasonoptionsis not supported.libraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.ts (3)
3-3: LGTM!The type import for
WritableStreamis correctly added to support the new stream-based API.
29-31: LGTM!The
streamgetter appropriately exposes the internalMediaStreamfor external use.
49-52: LGTM!The resize handler correctly synchronizes element dimensions with video frame dimensions, maintaining proper aspect ratio.
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (6)
2-2: LGTM!The
WritableStreamimport is correctly added to support the new stream-based rendering pipeline.
12-17: LGTM!The new private fields appropriately support the device-pixel rendering feature and frame lifecycle management.
37-77: Verify error handling for concurrent draw calls.The
useDevicePixelsfeature is well-implemented with proper runtime validation. However, on line 72,void this.draw(this.#frame)discards the promise, which could lead to unhandled rejections ifdraw()fails. Additionally, rapid resize events might trigger overlapping draw calls.Consider whether the draw operation should be awaited or if there should be debouncing/throttling for resize events.
79-92: LGTM!The
#updateSizemethod correctly clamps dimensions to the frame size and avoids unnecessary updates when dimensions haven't changed.
105-122: LGTM!The
Optionsinterface is well-documented with clear guidance on theuseDevicePixelsfeature, including performance implications and usage constraints.
96-102: No breaking changes from the return type modification.The
dispose()method is not part of theVideoFrameRendererinterface contract, nor does it implement the standardDisposableinterface which would requiredispose(): void. The current callers at lines 30-31 (stream handler callbacks) do not await the method, so the return type change toundefinedintroduces no compatibility issues.The cleanup logic properly disconnects the ResizeObserver and closes the saved frame.
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (5)
1-2: LGTM!The cspell directives correctly suppress false positives for GLSL precision qualifiers.
95-96: Verify context type narrowing is intentional.Line 95 narrows the context type to
WebGLRenderingContext, butglCreateContext(line 111) can returnWebGL2RenderingContext. While theinstanceofcheck on line 161 handles WebGL2 features at runtime, the type narrowing loses compile-time type safety for WebGL2-specific APIs.Consider using a union type to preserve type information:
#context: WebGLRenderingContext | WebGL2RenderingContext;
105-152: LGTM!The constructor properly integrates the options parameter, passing it to the parent class and using
enableCaptureto configure WebGL context attributes. The trade-off betweenpreserveDrawingBufferanddesynchronizedis well-balanced for capture vs. latency.The program creation uses the new helper function and is properly initialized with
useProgram.
169-191: LGTM!The
overridekeyword correctly indicates method inheritance. The addition ofgl.flush()ensures commands are submitted for execution, which can improve frame timing in streaming scenarios, especially with the desynchronized context setting.
193-213: LGTM!The disposal sequence correctly cleans up resources in the proper order: program deletion, context invalidation, then parent cleanup. The
Optionsinterface appropriately extends the parent options and adds theenableCaptureflag with clear documentation.
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.ts (1)
13-62: Missing cleanup logic for MediaStreamTrackGenerator.The class no longer has a
dispose()method after removing the lifecycle methods. TheMediaStreamTrackGeneratortrack should be stopped when no longer needed to release resources. Consider adding cleanup:🔧 Suggested addition
+ dispose(): undefined { + this.#generator.stop(); + this.#stream.getTracks().forEach(track => track.stop()); + }libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
212-223: Missingcontroller.terminate()in dispose.The
dispose()method clears internal state but doesn't terminate theTransformStream. Downstream consumers may hang waiting for more data. Consider terminating the controller:🔧 Suggested fix
dispose() { if (this.#disposed) { return; } this.#disposed = true; this.#pendingConfiguration = undefined; this.#pendingFrames.length = 0; this.#disposeVisibilityTracker?.(); + this.#controller.terminate(); }
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts:
- Around line 220-234: The TransformStream transform callback filters out frames
when frame.timestamp <= this.#skipFramesUntil but never closes them, leaking GPU
resources; update the transform function inside the TransformStream in
decoder.ts to call frame.close() for skipped VideoFrame instances (i.e., when
frame.timestamp <= this.#skipFramesUntil) before returning, and keep
controller.enqueue(frame) only for frames that are forwarded so that all skipped
frames are explicitly closed.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts:
- Around line 108-113: The abort() handler currently leaves #nextFrame open
which can keep a pending VideoFrame alive; after calling
this.#readableController.error(reason) and this.#counter.dispose() also
explicitly close/cancel #nextFrame to release any held VideoFrame and avoid
leaks (but keep the existing behavior of not closing #captureFrame so snapshots
still work); make the close/cancel call defensive (check for existence and
whether it's already closed) so it’s safe to call from abort and won’t interfere
with normal #draw() completion.
🧹 Nitpick comments (1)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
56-76: Consider error handling for resize redraw.The
void this.draw()on line 72 swallows any errors from the redraw operation. While this prevents unhandled promise rejections, errors during resize-triggered redraws will be silently lost.💡 Optional: Add error logging
if (this.#updateSize(width, height) && this.#frame) { - void this.draw(this.#frame); + void this.draw(this.#frame).catch((e) => { + console.error("Failed to redraw on resize:", e); + }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.tslibraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.tslibraries/scrcpy-decoder-webcodecs/src/video/render/type.tslibraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
🧬 Code graph analysis (3)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (8)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (3)
PauseController(12-224)Output(228-230)Input(227-227)libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (2)
RendererController(17-159)frame(117-136)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
CodecDecoder(7-10)Output(22-22)Input(13-15)libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (1)
H264Decoder(6-16)libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (1)
H265Decoder(6-19)libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (1)
Av1Codec(6-43)libraries/scrcpy-decoder-webcodecs/src/video/time.ts (1)
increasingNow(21-28)libraries/stream-extra/src/inspect.ts (1)
InspectStream(5-23)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
libraries/stream-extra/src/consumable.ts (1)
WritableStream(71-71)
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (7)
libraries/scrcpy-decoder-webcodecs/src/video/pool.ts (1)
Pool(1-37)libraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts (1)
VideoFrameCapturer(1-42)libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-tinyh264/src/types.ts (1)
ScrcpyVideoDecoderPerformanceCounter(15-36)libraries/stream-extra/src/push-readable.ts (2)
PushReadableStreamController(6-14)PushReadableStream(41-307)libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
PerformanceCounter(3-64)libraries/adb-scrcpy/src/client.ts (1)
controller(285-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (11)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
1-5: Clean stream-based interface design.The simplified interface exposing only
writable: WritableStream<VideoFrame>aligns well with the stream-based architecture and matches the standardTransformStream.writablenaming convention.libraries/scrcpy-decoder-webcodecs/src/video/render/insertable-stream.ts (1)
49-52: Good use of resize event for dimension synchronization.The resize event listener correctly syncs the video element's
width/heightattributes with its intrinsicvideoWidth/videoHeight, ensuring proper rendering dimensions.libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (2)
152-158: Proper cleanup in dispose.The
dispose()method correctly closes both#captureFrameand#nextFrame, preventing resource leaks when manually cleaning up.
78-94: Frame lifecycle is correct; no use-after-close issue.The write handler intentionally stores the original frame in
#nextFrame(line 92) without cloning because frame ownership transfers to the stream viaenqueue()in the#drawmethod. The#nextFramereference is cleared toundefined(line 129) before the frame is enqueued, so when subsequent writes arrive during drawing, they do not close an in-flight frame—they only replace the queued frame reference. The renderer receives ownership of the enqueued frame and is responsible for closing it. The pattern is sound:#captureFrameis cloned for snapshot safety, while#nextFramemaintains the original reference that transfers ownership downstream.libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (2)
50-102: Clean synchronous controller initialization.The controller capture pattern correctly assigns the controller synchronously in the
startcallback, avoiding the race condition from the previous implementation that usedawait Promise.resolve().
149-162: Empty array case handled correctly.The
for...ofiteration overentries()safely handles an empty#pendingFramesarray by simply not executing the loop body, avoiding the previous concern about accessingframes[-1].libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (2)
19-32: Good stream-based frame handling.The
WritableStreamproperly manages the frame lifecycle: closes the previous frame before storing the new one, conditionally updates size based on options, and delegates to the abstractdraw()method.
79-92: Well-designed size update logic.The
#updateSizemethod correctly clamps dimensions to avoid upscaling beyond source resolution and returns a boolean to enable conditional redraws, avoiding unnecessary work.libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (3)
177-240: Well-structured streaming pipeline.The pipeline composition is clean and readable with clear stage responsibilities:
- Timestamp injection
- Codec-specific packet transformation
- Configuration inspection/size tracking
- Raw video decoding
- Decode time tracking and skip filtering
- Render flow control
- Final rendering
The
void ... .catch(noop)pattern prevents unhandled rejections.
270-281: Clean options interface using type reuse.Using
Pick<VideoDecoderConfig, ...>ensures type consistency with the native WebCodecs API and avoids duplicating type definitions.
259-267: dispose() effectively halts pipeline processing through the#disposedflag.The dispose() method doesn't need to explicitly abort the writable stream. When
#pause.dispose()is called, it sets#disposed = true, which causes PauseController's transform function to return early without enqueueing packets (lines 58-59 and 83-84). This prevents any further data from flowing through the pipeline, even if the source stream continues sending data.The design is intentional: dispose() handles resource cleanup while stream closure is managed separately via the comment's note about cleanup happening "when
writableends."Likely an incorrect or invalid review comment.
yume-chan
changed the title
yume-chan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts:
- Around line 13-15: The comment next to the alpha: true setting in bitmap.ts
incorrectly cites WebGL guidance; update it to reference
bitmaprenderer/ImageBitmapRenderingContext guidance (e.g., MDN
ImageBitmapRenderingContext or canvas getContext options) and clarify why we
keep alpha: true here: if transparency is required we must use alpha: true,
whereas setting alpha: false can enable opaque-canvas optimizations for
bitmaprenderer — replace the WebGL link and/or add a short note explaining that
distinction next to the alpha: true setting.
🧹 Nitpick comments (1)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (1)
248-250: Consider simplifying the return type.The explicit
undefinedreturn type andreturnstatement is unusual. Consider usingvoidreturn type instead:♻️ Proposed simplification
- resume(): undefined { + resume(): void { this.#pause.resume(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
🧬 Code graph analysis (1)
libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (5)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (2)
canvas(8-10)Options(113-128)libraries/scrcpy-decoder-tinyh264/src/decoder.ts (2)
canvas(40-42)Options(264-271)libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (1)
Options(272-282)libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
Options(205-212)libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (1)
frame(117-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (6)
libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (2)
6-10: LGTM!Constructor signature update aligns with the
Options-based pattern used inCanvasVideoFrameRendererandWebGLVideoFrameRenderer.
19-23: The review comment is incorrect. Theframeparameter should NOT be closed in thedraw()method.The parent class
CanvasVideoFrameRendererexplicitly documents (line 99-100 in canvas.ts):"Derived classes must not transfer or close the
frame."The frame is intentionally kept alive by
CanvasVideoFrameRendererto support redraw on resize (stored in#framefield). The parent class manages the frame lifecycle by:
- Closing the previous frame when a new one arrives (line 21-22)
- Closing it when the stream closes or is disposed (line 108)
The comment in flow-control.ts about "the renderer is responsible for closing
frame" refers to theCanvasVideoFrameRendereras the owner of frame lifecycle, not the individualdraw()method implementation. No fix is needed.libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (4)
177-241: Verify error handling strategy in the streaming pipeline.The
.catch(noop)at line 241 silently swallows all pipeline errors, including configuration failures, decoder errors, and renderer failures. Based on learnings, configuration failures should propagate up the client pipeline rather than being suppressed.Consider whether errors should be:
- Logged for debugging
- Propagated through an error callback/event
- Exposed via a promise that clients can monitor
This is especially important for configuration errors during mid-stream encoder restarts.
260-268: LGTM: Dispose method aligns with streaming lifecycle.The dispose implementation correctly relies on automatic stream cleanup when the writable ends, with explicit cleanup only for resources that persist beyond the stream lifecycle (size tracking, pause controller, render controller).
272-275: LGTM: Clean interface design.The Options interface appropriately extends
VideoDecoderConfigusingPickto inherit the relevant hardware acceleration and latency optimization settings, which are then applied to the config in the pipeline (lines 210-211).
190-190: No timing issue:increasingNow()is built directly onperformance.now()and ensures monotonic values while preserving the same time base. The subtraction at line 225 (performance.now() - frame.timestamp) produces correct decode time calculations.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts:
- Around line 177-241: The final .pipeTo(renderer.writable).catch(noop) silently
swallows all pipeline errors; remove the noop swallow and instead propagate or
surface errors: replace the trailing .catch(noop) with either returning the
Promise from pipeTo so callers can await it, or attach a handler that logs and
re-throws (e.g., .catch(err => { processError(err); throw err; })), so failures
from the stream chain (starting from this.#pause through codecDecoder,
InspectStream, this.#rawDecoder, this.#renderController to renderer.writable)
are not suppressed; ensure whichever method you pick surfaces errors to the
consumer or options-provided error callback.
- Around line 104-115: The code is repurposing
EncodedVideoChunk.timestamp/VideoFrame.timestamp to store local processing time
which violates WebCodecs semantics; stop writing processing milliseconds into
EncodedVideoChunk.timestamp and instead track per-frame processing times using a
separate structure (e.g., a WeakMap from VideoFrame/EncodedVideoChunk to timing
or an internal field on your decoder instance), update any logic that reads
timestamps to use the new timing storage, and keep the existing get
totalDecodeTime() (#totalDecodeTime) aggregate unchanged so presentation
timestamps remain untouched and in microseconds.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/fallback.ts:
- Around line 6-26: The constructor of AutoRenderer currently types the options
parameter as WebGLVideoFrameRenderer.Options but passes it to both
WebGLVideoFrameRenderer and BitmapVideoFrameRenderer; change the constructor
signature in AutoRenderer to accept options: CanvasVideoFrameRenderer.Options so
the type matches what both renderers accept (WebGLVideoFrameRenderer will still
accept the base options since it extends them); update the constructor parameter
type only (constructor(canvas?: HTMLCanvasElement | OffscreenCanvas, options?:
CanvasVideoFrameRenderer.Options)) and leave the existing logic that
instantiates WebGLVideoFrameRenderer or BitmapVideoFrameRenderer unchanged.
🧹 Nitpick comments (3)
libraries/scrcpy-decoder-webcodecs/src/video/render/fallback.ts (1)
28-30: Use optional chaining for dispose call.Since
disposeis optional in theVideoFrameRendererinterface (line 7 in type.ts), callingthis.#inner.dispose()directly could throw if the inner renderer doesn't implement it.🛡️ Proposed fix using optional chaining
dispose() { - return this.#inner.dispose(); + return this.#inner.dispose?.(); }libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (2)
177-241: Consider extracting pipeline construction to helper methods.The constructor contains a deeply nested pipeline with multiple transformation stages spanning 60+ lines. Extracting logical stages into private helper methods would improve readability and testability.
💡 Example refactoring approach
constructor(options: WebCodecsVideoDecoder.Options) { // ... initialization ... void this.#pause.readable .pipeThrough(this.#createTimestampTransform()) .pipeThrough(codecDecoder) .pipeThrough(this.#createConfigInspector(hardwareAcceleration, optimizeForLatency)) .pipeThrough(this.#rawDecoder) .pipeThrough(this.#createDecodeTimeTracker()) .pipeThrough(this.#renderController) .pipeTo(renderer.writable) .catch(noop); } #createTimestampTransform() { return new TransformStream<PauseController.Output, CodecDecoder.Input>({ transform: (packet, controller) => { // ... existing logic ... }, }); } #createConfigInspector(hardwareAcceleration: string, optimizeForLatency: boolean) { return new InspectStream((chunk): undefined => { // ... existing logic ... }); } #createDecodeTimeTracker() { return new TransformStream<VideoFrame, VideoFrame>({ transform: (frame, controller) => { // ... existing logic ... }, }); }
248-250: Unusual explicitundefinedreturn type.The
resume()method explicitly returnsundefinedrather thanvoid. While this matches TypeScript's behavior (void functions return undefined at runtime), the conventional practice is to usevoidfor methods without meaningful return values. This likely matches theScrcpyVideoDecoderinterface signature.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/fallback.tslibraries/scrcpy-decoder-webcodecs/src/video/render/index.tslibraries/scrcpy-decoder-webcodecs/src/video/render/type.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-webcodecs/src/video/render/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
🧬 Code graph analysis (1)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (6)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (3)
PauseController(12-224)Output(228-230)Input(227-227)libraries/scrcpy-decoder-webcodecs/src/video/video-decoder-stream.ts (1)
VideoDecoderStream(4-104)libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (2)
RendererController(17-159)frame(117-136)libraries/scrcpy-decoder-webcodecs/src/video/render/fallback.ts (1)
AutoRenderer(6-31)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (3)
CodecDecoder(7-10)Output(22-22)Input(13-15)libraries/scrcpy-decoder-webcodecs/src/video/time.ts (1)
increasingNow(21-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (8)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
4-8: LGTM! Clean stream-based API.The transition to a
WritableStream<VideoFrame>property provides a cleaner, more composable API compared to the previous imperative method-based approach. Makingdisposeoptional is reasonable for flexibility.libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (7)
42-57: Well-designed pause control mechanism.The pause controller setup with
#skipFramesUntilprovides a clean mechanism for coordinating frame skipping across the pipeline. The timestamp-based approach correctly marks the boundary between frames to skip and frames to process.
74-102: Clean delegation to VideoDecoderStream.The raw decoder setup correctly exposes VideoDecoderStream metrics through well-documented getters. The naming convention (e.g.,
framesSkippedDecodingvsframesSkippedRendering) clearly distinguishes between different pipeline stages.
117-147: Comprehensive renderer metrics with helpful documentation.The renderer controller setup provides clear metrics tracking with excellent JSDoc. The note about the Chrome bug for
framesPresentedin nested Web Workers is particularly valuable for debugging potential issues.
256-258: Clean delegation for snapshot functionality.The
snapshot()method correctly delegates to the render controller, allowing callers to capture the current rendered frame state. Errors propagate naturally to the caller.
273-283: Type-safe Options interface using Pick.The Options interface correctly uses
Pick<VideoDecoderConfig>to ensure type compatibility with the WebCodecs API. This prevents type drift if the VideoDecoderConfig interface changes in the future.
6-6: No action required.The imports of
PauseControllerandnoopfrom@yume-chan/scrcpy-decoder-tinyh264are appropriate. PauseController is a decoder-specific abstraction that handles pause/resume logic, pending frame buffering, and document visibility tracking for video decoding—not a general utility. The webcodecs decoder intentionally builds on this tinyh264 abstraction, and no circular dependency exists (tinyh264 does not depend on webcodecs). The current architecture is sound.Likely an incorrect or invalid review comment.
260-269: No changes needed. The code correctly handles#rawDecodercleanup through the stream pipeline lifecycle.When
renderer.writableends (either through normal completion or error), thepipeTo()operation automatically cancels all upstream transforms in the chain, including#rawDecoder. This invokesVideoDecoderStream.cancel(), which callsdecoder.close(). The comment explaining that "cleanup happens automatically whenwritableends" is accurate and the current implementation is correct.
libraries/scrcpy-decoder-webcodecs/src/video/render/fallback.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts (1)
74-76: Consider explicit event listener cleanup.The "dequeue" event listener is never explicitly removed. While the decoder being closed prevents further events, using an
AbortControlleror storing the listener reference for explicit removal would be cleaner.♻️ Optional: Use AbortController for cleanup
+ #dequeueAbortController = new AbortController(); + constructor() { let decoder!: VideoDecoder; super({ start: (controller) => { decoder = new VideoDecoder({ output: (frame) => { this.#framesDecoded += 1; controller.enqueue(frame); }, error: (error) => { controller.error(error); this.#onDequeue.dispose(); + this.#dequeueAbortController.abort(); }, }); - decoder.addEventListener("dequeue", () => - this.#onDequeue.fire(undefined), + decoder.addEventListener( + "dequeue", + () => this.#onDequeue.fire(undefined), + { signal: this.#dequeueAbortController.signal }, ); }, // ... in flush and cancel: + // this.#dequeueAbortController.abort();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
🧬 Code graph analysis (1)
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts (2)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (7)
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts (7)
4-7: Clean API design with dual-purpose input type.Using a union type for configuration and encoded chunks allows a single stream to handle both setup and data flow elegantly.
8-27: Well-documented state invariants.The documentation clearly explains why the callbacks don't need to check for "closed" state, making the code easier to maintain. The definite assignment pattern is safe since
startruns synchronously.
29-35: Good encapsulation of event emission.Exposing only the
eventproperty prevents external code from firing events while still allowing subscriptions.
37-51: Good observability metrics.These counters provide useful visibility into decoder performance and latency management behavior.
53-56: LGTM!The saved configuration enables proper decoder reset behavior.
78-104: Solid latency management strategy.The keyframe-based reset logic effectively bounds latency by discarding pending frames when the decoder can't keep up. The comment clearly explains the rationale. The non-null assertion on
#configat line 100 is safe since the unconfigured state check on line 85 guarantees prior configuration.
105-124: Proper shutdown handling for both graceful and abrupt termination.The
flushcallback correctly awaits pending decodes for graceful shutdown, whilecancelimmediately closes the decoder for error scenarios. The state check before flushing handles the edge case where no configuration was ever sent.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts:
- Around line 199-200: Typo in the inline comment: change "clsoed" to "closed"
above the assignment to the private field this.#lastFrame in webgl.ts; update
the comment to read something like "// Will be closed by
`CanvasVideoFrameRenderer`" next to the this.#lastFrame = frame; assignment to
fix spelling and clarify intent referencing CanvasVideoFrameRenderer and the
#lastFrame field.
🧹 Nitpick comments (1)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
79-81: Potential unhandled promise rejection in resize callback.The
void this.draw(...)call discards the promise result. Ifdraw()throws, the error will become an unhandled rejection. Consider wrapping in a.catch()handler.Suggested fix
if (this.#updateSize(width, height) && this.#lastFrame) { - void this.draw(this.#lastFrame); + void this.draw(this.#lastFrame).catch(() => { + // Ignore errors during resize redraw + }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/auto-canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/index.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.tslibraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/render/auto-canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/index.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
📚 Learning: 2025-08-14T11:06:21.851Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 780
File: libraries/adb-credential-web/src/storage/index.ts:1-5
Timestamp: 2025-08-14T11:06:21.851Z
Learning: The yume-chan/adb-credential-web package doesn't use "exports" in package.json for better compatibility. Instead, they re-export everything from the main entrypoint rather than using subpath exports.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/render/index.ts
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
🧬 Code graph analysis (3)
libraries/scrcpy-decoder-webcodecs/src/video/render/auto-canvas.ts (5)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
VideoFrameRenderer(4-8)libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (3)
canvas(11-13)options(16-18)Options(119-134)libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (2)
WebGLVideoFrameRenderer(62-247)Options(250-257)libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (1)
Options(227-237)libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (1)
BitmapVideoFrameRenderer(3-24)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (10)
libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (1)
PauseController(12-224)libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts (1)
VideoDecoderStream(4-136)libraries/scrcpy-decoder-webcodecs/src/video/utils/timestamp.ts (1)
TimestampTransforms(35-99)libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (1)
RendererController(17-159)libraries/scrcpy-decoder-webcodecs/src/video/render/auto-canvas.ts (1)
AutoCanvasRenderer(6-31)libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
Options(119-134)libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
Options(250-257)libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (1)
H264TransformStream(6-16)libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (1)
H265TransformStream(6-19)libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (1)
Av1TransformStream(6-46)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (2)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (2)
Options(119-134)canvas(11-13)libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (2)
glCreateContext(11-59)glLoseContext(61-67)
🔇 Additional comments (16)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (4)
25-40: WritableStream lifecycle looks correct.The
writecallback properly closes the previous frame before storing the new one, andclose/abortboth calldispose()which handles cleanup of the ResizeObserver and last frame. This aligns with the learning about explicit cleanup in stream-based code paths.
64-84: Non-null assertions on ResizeObserverEntry arrays.The code uses non-null assertions (
!) onentries[0],entry.devicePixelContentBoxSize[0], andentry.contentBoxSize[0]. While these should always be populated for aResizeObservercallback, the defensive pattern would be to guard against empty arrays.However, given the ResizeObserver spec guarantees at least one entry and one size box for observed elements, these assertions are safe in practice.
87-100:#updateSizeclamps to frame dimensions correctly.The method properly clamps the canvas size to the frame's coded dimensions when a frame is present, preventing the canvas from being larger than the video content. The boolean return value enables callers to know if a redraw is needed.
109-115:dispose()cleanup is thorough.The method properly disconnects the ResizeObserver, closes the last frame, and resets canvas dimensions. This explicit cleanup aligns with the retrieved learning about not relying on stream cancel callbacks for cleanup.
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (4)
14-19:createShaderlacks compilation error checking.The helper creates and compiles a shader but doesn't verify
COMPILE_STATUS. WhilecreateProgramchecks this later if linking fails, returning an invalid shader object could lead to confusing error messages.This is acceptable since the best practice (per MDN link in code) is to defer shader compilation checks until link failure, which the code follows.
38-59: Robust shader/program error handling with eager cleanup.The
createProgramfunction follows WebGL best practices:
- Defers shader compile status checks until link failure
- Eagerly deletes shader objects in the
finallyblock- Provides meaningful error messages from info logs
186-196: Context loss/restoration handling is well-implemented.The handler prevents default behavior to signal intent to restore, and
#handleContextRestoredproperly re-initializes the GL program and redraws the last frame if available. This ensures rendering resumes correctly after context recovery.
229-246:dispose()cleanup is comprehensive.The method properly:
- Deletes the GL program
- Removes event listeners (preventing memory leaks)
- Loses the context eagerly (per MDN best practices)
- Calls
super.dispose()for base class cleanupThis follows the retrieved learning about explicit cleanup in stream-based code paths.
libraries/scrcpy-decoder-webcodecs/src/video/render/index.ts (1)
1-9: New module exports look good.The additions of
auto-canvas.jsandflow-control.jsexports are consistent with the existing barrel pattern. Based on learnings, this package re-exports everything from the main entrypoint rather than using subpath exports.libraries/scrcpy-decoder-webcodecs/src/video/render/auto-canvas.ts (2)
6-15: Delegate pattern implementation is clean.The
AutoCanvasRendererproperly implementsVideoFrameRendererby delegating to the inner renderer. Thecanvasandwritablegetters provide transparent access to the underlying implementation.
28-30:dispose()delegates correctly to inner renderer.The method properly forwards disposal to the inner renderer, which handles its own cleanup including the base class cleanup chain.
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (5)
145-168: Constructor options destructuring with defaults is clean.The default values for
renderer,hardwareAcceleration, andoptimizeForLatencyare sensible:
AutoCanvasRenderer()provides automatic WebGL/Bitmap selection"no-preference"for hardware acceleration is a safe defaulttrueforoptimizeForLatencyprioritizes low latency over quality, appropriate for screen mirroringThe codec-to-transform-stream mapping is exhaustive with proper error handling for unsupported codecs.
176-184: MutatingchunkinInspectStreamcallback.The
InspectStreamcallback mutates thechunkobject by addinghardwareAccelerationandoptimizeForLatencyproperties. While this works, it modifies objects flowing through the stream. This is intentional to inject decoder configuration, but worth noting for maintainability.
226-237: Options interface extendsPick<VideoDecoderConfig, ...>appropriately.The options type correctly picks
hardwareAccelerationandoptimizeForLatencyfrom the standardVideoDecoderConfigtype, ensuring type compatibility with the WebCodecs API.
170-196: Pipeline errors are suppressed without consumer notification.The
.catch(noop)on line 195 swallows the promise rejection from.pipeTo(), preventing consumers from observing any errors that occur in the decoding or rendering pipeline. While errors do internally propagate through the streams (viacontroller.error()), theWebCodecsVideoDecoderclass exposes noonErrorcallback or error event, leaving consumers unable to detect or respond to failures.Consider exposing an error event or callback so consumers can react to decoding and rendering failures.
214-223: Consider explicitly canceling the stream pipeline indispose()to ensure proper cleanup.The method cleans up
#size,#pause,#renderController, and#renderer, but doesn't explicitly cancel the underlying streams. While the comment states that cleanup happens whenwritableends, if the caller never closesdecoder.writable(the return value of#pause.readableafter pipeThrough), the stream pipeline never closes and theflush()andcancel()callbacks in VideoDecoderStream never run. This leaves the native VideoDecoder resource improperly cleaned up.Explicitly calling
.cancel()on the stream at the start of the pipeline would ensure all downstream cleanup callbacks execute, even if the caller hasn't closed the writable end.⛔ Skipped due to learnings
Learnt from: yume-chan Repo: yume-chan/ya-webadb PR: 827 File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120 Timestamp: 2026-01-12T06:58:19.519Z Learning: In TransformStream implementations, calling `controller.error()` immediately clears the internal `[[flushAlgorithm]]` and `[[cancelAlgorithm]]` slots, which prevents the `flush()` and `cancel()` callbacks from being invoked. This means that if a decoder's error callback calls `controller.error()`, the stream's `cancel()` callback will not subsequently run, eliminating concerns about double-close scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (8)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (8)
1-12: LGTM!Clean imports and the pre-resolved promise constant is an efficient pattern for methods that complete synchronously but need to return a Promise.
14-19: LGTM!The design correctly defers compile status checking to
createProgram(only when linking fails), following WebGL best practices as documented in the MDN reference.
21-60: Good implementation following WebGL best practices.The deferred shader compile checking and eager shader deletion align with MDN recommendations. One minor consideration:
gl.createProgram()on line 33 can returnnullif the context is lost, which would causeattachShaderto generate a WebGL error. However, since this is called during initialization (either in constructor after context validation or during context restoration), this edge case is unlikely to be hit in practice.
62-93: LGTM!The shader sources correctly implement a simple texture rendering pipeline with proper UV coordinate mapping. The
isSupportedcheck appropriately disallows software rendering.
95-145: LGTM!Good implementation with appropriate WebGL context options:
preserveDrawingBufferanddesynchronizedtrade-offs based on capture needs are well-documented.- Context validation before proceeding is proper.
- Event listeners are correctly paired with removal in
dispose().
147-184: LGTM!The initialization correctly sets up the WebGL pipeline. The texture parameters (NEAREST filtering, CLAMP_TO_EDGE) are appropriate for video frame rendering. The WebGL1/WebGL2 conditional for mipmap support is a nice touch.
198-227: LGTM!Solid implementation:
- Early exit on context loss prevents WebGL errors.
gl.flush()ensures commands are submitted to the GPU.- Setting viewport on each draw correctly handles canvas resize scenarios.
229-258: LGTM!Proper resource cleanup following MDN best practices:
- Event listeners removed before context loss.
glLoseContextcalled eagerly to free GPU resources.super.dispose()called appropriately at the end.The
Optionsinterface is a clean extension with well-documentedenableCaptureoption.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:
- Around line 193-200: The code is incorrectly creating an EncodedAudioChunk for
video frames; in the method that calls this.#decoder.decode(...) inside
VideoDecoderStream (or the class handling video decoding), replace new
EncodedAudioChunk({...}) with new EncodedVideoChunk({...}) and keep the same
properties (type, timestamp, duration, data); ensure EncodedVideoChunk is
available in scope (import or global) so the decoder receives the correct chunk
type for video decoding.
- Around line 163-171: The code constructs an EncodedAudioChunk for video
frames, causing runtime failures; update the construction in the call to
this.#decoder.decode to create an EncodedVideoChunk instead, keeping the same
fields (use chunk.type ?? "key", chunk.timestamp, chunk.duration, chunk.data)
and ensure any typings/imports/uses around EncodedVideoChunk are corrected
(replace EncodedAudioChunk with EncodedVideoChunk wherever this decoding path is
used so the VideoDecoder receives the proper chunk type).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h264.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h265.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/type.tslibraries/scrcpy-decoder-webcodecs/src/video/utils/frame-type.tslibraries/scrcpy-decoder-webcodecs/src/video/utils/index.tslibraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.tslibraries/stream-extra/src/concat.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-webcodecs/src/video/utils/index.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/utils/frame-type.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/av1.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h265.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/type.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h264.tslibraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h264.tslibraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
📚 Learning: 2025-08-14T11:20:01.711Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 780
File: libraries/adb-credential-web/src/storage/prf/source.ts:2-10
Timestamp: 2025-08-14T11:20:01.711Z
Learning: TypeScript 5.9 introduced generic TypedArrays (e.g., Uint8Array<ArrayBuffer>, Int32Array<SharedArrayBuffer>) to preserve the distinction between ArrayBuffer and SharedArrayBuffer at the type level. The syntax Uint8Array<ArrayBuffer> is valid and recommended in TypeScript 5.9+ for type safety. This change helps prevent unsafe mixing of ArrayBuffer and SharedArrayBuffer types, especially important for WebCrypto APIs and other browser APIs that have different behavior for these buffer types.
Applied to files:
libraries/stream-extra/src/concat.ts
🧬 Code graph analysis (5)
libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (4)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
Input(20-22)Output(39-39)libraries/media-codec/src/av1.ts (1)
Av1(197-762)libraries/scrcpy-decoder-webcodecs/src/video/utils/frame-type.ts (1)
convertFrameType(1-7)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (2)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (2)
data(33-44)Config(48-48)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (1)
Config(24-35)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (4)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (4)
CodecTransformStream(7-10)Input(20-22)Output(39-39)Config(24-35)libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts (2)
Input(227-227)Output(228-230)libraries/scrcpy-decoder-webcodecs/src/video/utils/frame-type.ts (1)
convertFrameType(1-7)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (3)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (2)
data(33-44)Config(48-48)libraries/adb-scrcpy/src/video.ts (3)
data(76-79)data(81-84)data(86-100)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (1)
Config(24-35)
libraries/stream-extra/src/concat.ts (1)
libraries/struct/src/buffer.ts (2)
buffer(126-332)EmptyUint8Array(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (11)
libraries/stream-extra/src/concat.ts (2)
138-152: LGTM!The function correctly handles all three input types:
- Passthrough for existing
Uint8Array- Proper view creation with
byteOffset/byteLengthforArrayBufferView- Direct wrapping for raw
ArrayBufferLikeThe view semantics (no copy) is consistent with the existing
concatUint8Arraysbehavior.
154-172: LGTM!Clean implementation that mirrors the established
concatUint8Arrayspattern while supporting broader buffer types. The optimizations for 0/1 element cases and consistent use ofbyteLengthare appropriate.libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (2)
7-29: LGTM! Clean abstract TransformStream implementation.The transform correctly distinguishes configuration vs data packets and delegates codec-specific configuration parsing to subclasses. The
#configurewrapper appropriately adds therawfield for Annex B format handling.
47-49: LGTM!The
Configtype correctly omitsrawsince it's automatically added by the base class's#configuremethod, preventing accidental duplication.libraries/scrcpy-decoder-webcodecs/src/video/utils/frame-type.ts (1)
1-7: LGTM!Simple, well-typed utility function that correctly maps the keyframe boolean to WebCodecs'
EncodedVideoChunkType.libraries/scrcpy-decoder-webcodecs/src/video/codec/h265.ts (1)
5-17: LGTM!Clean implementation of
H265TransformStream. Theconfiguremethod correctly returns the parsed configuration with cropped dimensions as required by Edge's WebCodecs implementation.libraries/scrcpy-decoder-webcodecs/src/video/codec/av1.ts (1)
8-45: LGTM! AV1 transform stream correctly handles inline configuration.Unlike H.264/H.265, AV1's configuration is embedded in the bitstream via sequence headers. The implementation correctly:
- Ignores separate configuration packets (not used by AV1)
- Searches each data packet for sequence headers
- Emits configuration when a sequence header is found, followed by the video chunk
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts (2)
82-86: LGTM! Error handling correctly follows TransformStream cleanup pattern.Per the learning about
controller.error()clearing the cancel callback, the error handler appropriately calls#dispose()directly before the error propagates, ensuring resources are cleaned up regardless of whether cancel is invoked.
123-138: LGTM! Core frame-skipping logic is well-designed.The keyframe-based reset correctly implements the PR's goal of limiting latency when the decoder can't keep up. Resetting on keyframe arrival (when queue is non-empty) bounds maximum latency to one keyframe interval while ensuring decoding can resume from the new keyframe.
libraries/scrcpy-decoder-webcodecs/src/video/codec/h264.ts (1)
5-15: LGTM!Clean implementation mirroring
H265TransformStream. Theconfiguremethod correctly parses H.264 configuration and returns the required dimensions.libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (1)
1-44: LGTM! Well-structured type definitions for the TransformStream-based codec architecture.The type system cleanly models:
- Input packets (configuration or timestamped data)
- Output chunks (decoder config or video frames)
- The optional
rawbuffer for Annex B codecs (H.264/H.265)The
Optionalutility type properly handles the case whereEncodedVideoChunkInit.typemay be unknown initially.
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
Show resolved
Hide resolved
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts (2)
73-87: Clarify the comment aboutthisusage.The comment on line 75 states "can't use
thishere" but the nested callbacks on lines 79 and 85 do accessthis. This works because those callbacks are invoked asynchronously after the constructor completes. Consider clarifying the comment to avoid confusion:start: (controller) => { - // WARN: can't use `this` here + // WARN: can't use `this` synchronously here, + // but async callbacks (output/error) are safe.
174-201: Consider consolidating the two decode branches.The two decode calls (lines 182-189 and 193-200) differ only in whether
config.rawis prepended to the data. This could be simplified:♻️ Optional refactor
#configureAndDecodeFirstKeyFrame( config: CodecTransformStream.Config, chunk: CodecTransformStream.VideoChunk, ) { this.#decoder.configure(config); this.#configured = true; - if (config.raw) { - this.#decoder.decode( - new EncodedVideoChunk({ - type: "key", - timestamp: chunk.timestamp, - duration: chunk.duration!, - data: concatBuffers([config.raw, chunk.data]), - }), - ); - return; - } - this.#decoder.decode( new EncodedVideoChunk({ type: "key", timestamp: chunk.timestamp, duration: chunk.duration!, - data: chunk.data, + data: config.raw + ? concatBuffers([config.raw, chunk.data]) + : chunk.data, }), ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts
🧬 Code graph analysis (1)
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts (5)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (2)
CodecTransformStream(7-10)Config(24-35)libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (1)
Config(48-48)libraries/event/src/event-emitter.ts (1)
EventEmitter(12-61)libraries/stream-extra/src/concat.ts (1)
concatBuffers(154-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (6)
libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts (6)
1-9: LGTM!The imports are appropriate and the class type signature correctly models the input (Config or VideoChunk) and output (VideoFrame) types for the transform stream.
10-29: LGTM!The comment explaining the decoder state management is valuable. The
#decoder!definite assignment is valid since thestartcallback assignsdecodersynchronously during thesuper()call, and line 114 captures it afterward.
31-68: LGTM!Good observability surface with
framesDecoded,framesSkipped, anddecoderResetCountgetters. The#configuredflag appropriately tracks whether the decoder has been configured with the current#config, which is necessary for handling reconfigurations separately from decoder state.
89-112: LGTM!The transform/flush/cancel callbacks are well-structured. The error handler correctly performs explicit cleanup via
#dispose()sincecontroller.error()preventscancel()from being invoked. Based on learnings, this is the correct pattern for TransformStream error handling.
163-171: Verify handling of undefined frame type after decoder is configured.When
this.#configuredis true andchunk.type !== "key", this path treatsundefinedtype as"key"(line 166). If the frame is actually a delta frame with an undefined type marker, passing it as"key"may cause the decoder to produce corrupted output or throw.Is this a known edge case from the upstream codec transform streams, or should undefined-typed frames be rejected at this point?
203-214: LGTM!The cleanup logic is thorough: removes the native event listener, disposes the EventEmitter, and defensively checks decoder state before closing. Using an arrow function for
#handleDequeueensures correctthisbinding when used as an event listener.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts:
- Around line 22-26: packet.keyframe is optional so
convertFrameType(packet.keyframe) may return undefined and end up passing an
invalid undefined `type` to EncodedVideoChunk; update the enqueue call in the
transform that calls controller.enqueue to ensure a valid type by using a
defensive fallback (e.g., set type to convertFrameType(packet.keyframe) ??
"delta") or alternatively make ScrcpyMediaStreamDataPacket.keyframe required in
CodecTransformStream.Input and update callers; reference the call site using
controller.enqueue and the convertFrameType helper to locate where to apply the
fix.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts:
- Around line 140-172: The RedrawController callback currently returns nothing
and must explicitly return undefined to satisfy MaybePromiseLike<undefined>;
update the lambda passed to the RedrawController constructor (the anonymous
(frame) => { ... } callback where texImage2D, uniform updates, viewport and
drawArrays are done) to end with an explicit return undefined so the function
signature matches MaybePromiseLike<undefined>.
🧹 Nitpick comments (1)
libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (1)
84-90: Consider rethrowing the error in redraw for consistency with draw().In
draw(), exceptions are both captured and rethrown. Here inredraw(), the error is captured but silently swallowed. Whileredraw()returnsvoid(so callers can't await it), the inconsistency could make debugging harder. If this is intentional because redraw is fire-and-forget, a brief comment would clarify.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/settings.jsonlibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/redraw.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/redraw.tslibraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts
🧬 Code graph analysis (3)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (4)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
Options(288-295)libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
VideoFrameRenderer(4-8)libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (1)
RedrawController(6-99)libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (1)
createCanvas(1-9)
libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (4)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
frame(85-99)libraries/stream-extra/src/stream.ts (1)
AbortController(13-23)libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts (1)
draw(22-26)libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
draw(263-265)
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (3)
libraries/stream-extra/src/stream.ts (1)
TransformStream(41-41)libraries/scrcpy-decoder-webcodecs/src/video/codec/type.ts (4)
CodecTransformStream(7-10)Input(20-22)Output(39-39)Config(24-35)libraries/scrcpy-decoder-webcodecs/src/video/utils/frame-type.ts (1)
convertFrameType(1-7)
🪛 GitHub Actions: Pull Request
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
[error] 140-140: TS2345: Argument of type '(frame: VideoFrame) => void' is not assignable to parameter of type '(frame: VideoFrame) => MaybePromiseLike'.
🔇 Additional comments (11)
.vscode/settings.json (1)
61-67: LGTM!The additions are valid technical terms that align with the PR's WebCodecs and WebGL context handling changes. The words are correctly placed in alphabetical order within the array.
libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (2)
29-53: LGTM - Well-structured draw method with proper frame lifecycle management.The promise chaining ensures sequential execution, and the frame is properly closed in the
finallyblock after drawing.
94-98: LGTM - Dispose properly cleans up resources.The method correctly closes the last frame, aborts pending redraws, and sets an error state to prevent further operations.
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (3)
27-33: LGTM - Clean RedrawController integration.The controller correctly handles size updates for non-external canvas modes before delegating to the abstract
drawmethod.
62-80: LGTM - ResizeObserver callback handles both modern and fallback sizing.The non-null assertions are safe per the ResizeObserver spec. The fallback from
devicePixelContentBoxSizetocontentBoxSizewith manualdevicePixelRatiocalculation is correctly implemented.
108-116: LGTM - Comprehensive dispose implementation.The method properly cleans up the canvas size, disconnects the ResizeObserver, and disposes the controller. This aligns with the learning about explicit cleanup in stream-based code.
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (3)
13-59: LGTM - Shader creation follows WebGL best practices.The deferred compile status check and eager shader deletion align with MDN WebGL best practices. The error handling properly surfaces shader/program errors with info logs.
253-261: LGTM - WebGL context loss/restoration properly handled.The
preventDefault()on context lost enables restoration, and the restored handler correctly reinitializes the program and triggers a redraw.
267-284: LGTM - Thorough WebGL resource cleanup.The dispose method properly deletes the program, removes event listeners, loses the context eagerly per MDN best practices, and delegates to the parent class for controller cleanup.
libraries/scrcpy-decoder-webcodecs/src/video/codec/h26x.ts (2)
31-42: Clean configuration pattern with good documentation.The separation between the abstract
configure(codec-specific) and private#configure(adds raw data for Annex B format) is well-structured. The inline comment and spec link provide helpful context for future maintainers.
45-47: LGTM!The namespace correctly exports a
Configtype that omitsraw, since the base class handles adding the raw buffer internally. This provides a clean contract for subclass implementations.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts:
- Around line 289-294: The JSDoc for the enableCapture property has a
grammatical error; update the description string for the enableCapture optional
property (in the WebGL renderer type/interface in webgl.ts) from "Enable this
option may reduce performance" to "Enabling this option may reduce performance"
so the comment reads correctly and clearly warns about potential performance
impact.
- Around line 32-35: gl.createProgram() may return null; check the returned
program before using it. After calling gl.createProgram(), verify that the
`program` is non-null and if it is null, throw or return an appropriate
error/early-exit (e.g., throw new Error("Failed to create WebGL program"))
instead of calling gl.attachShader/gl.linkProgram on a null value; then proceed
to call gl.attachShader(program, vertexShader) and gl.attachShader(program,
fragmentShader) and gl.linkProgram(program) only when `program` is valid.
- Around line 13-18: The createShader function uses a non-null assertion on
gl.createShader which can return null if the WebGL context is lost or the type
is invalid; change createShader to check the returned shader for null and handle
failure by throwing a descriptive Error (including type and gl.getError() if
helpful) or returning null, and also verify compilation succeeded with
gl.getShaderParameter(shader, gl.COMPILE_STATUS) and include
gl.getShaderInfoLog(shader) in the thrown error/log; update any callers of
createShader to handle the new error/nullable return accordingly.
🧹 Nitpick comments (2)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (2)
98-110: Nitpick:gaussian9samples only 5 texels, not 9.The function name suggests a 9-tap Gaussian kernel, but it only samples the center and 4 cardinal neighbors (5 taps). Consider renaming to
gaussian5or adding diagonal samples for a true 9-tap kernel.
155-166: Cache uniform locations during initialization.
getUniformLocationis called on every frame draw. While not expensive, caching these during#initializeis a WebGL best practice for better performance.Suggested approach
#program!: WebGLProgram; +#texelSizeLocation!: WebGLUniformLocation | null; +#zoomLocation!: WebGLUniformLocation | null; // In #initialize(): gl.useProgram(this.#program); +this.#texelSizeLocation = gl.getUniformLocation(this.#program, "texelSize"); +this.#zoomLocation = gl.getUniformLocation(this.#program, "zoom"); // In RedrawController callback: -const texelSizeLocation = gl.getUniformLocation(this.#program, "texelSize"); -gl.uniform2f(texelSizeLocation, ...); +gl.uniform2f(this.#texelSizeLocation, ...);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (5)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (5)
181-219: LGTM!The constructor follows WebGL best practices with appropriate context options for video rendering. The context lost/restored event handlers are properly set up using stable arrow function references for later removal.
221-251: LGTM!The initialization correctly sets up the WebGL pipeline. Using
NEARESTfiltering is appropriate since the fragment shader handles custom filtering based on zoom level.
253-261: LGTM!Context loss handling follows the standard pattern:
preventDefault()signals intent to restore, and#handleContextRestoredproperly reinitializes the pipeline and redraws the last frame for seamless recovery.
263-265: LGTM!Clean delegation to the
RedrawControllerwhich handles frame scheduling and the actual rendering.
267-284: Callthis.#controller.dispose()to prevent resource leaks.The
#controllerinstance holds references to aVideoFrameand anAbortControllerthat must be explicitly cleaned up. Itsdispose()method closes the frame and aborts any pending redraw operation. Addthis.#controller.dispose();before callingsuper.dispose().
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts:
- Around line 32-35: gl.createProgram() may return null; add a null check after
calling gl.createProgram() (the variable program used with gl.attachShader and
gl.linkProgram). If program is null, handle the failure immediately (e.g.,
log/processLogger.error and return or throw an Error) before calling
gl.attachShader(program, ...) or gl.linkProgram(program) to avoid silent
failures when the GL context is lost. Ensure references to program in functions
that follow are guarded so they won't be used when creation failed.
🧹 Nitpick comments (3)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (3)
13-18: Consider adding null check for shader creation.
gl.createShader()returnsnullif the context is lost or the type is invalid. The non-null assertion could mask issues. Consider adding a null check for defensive programming.♻️ Suggested improvement
function createShader(gl: WebGLRenderingContext, type: number, source: string) { const shader = gl.createShader(type)!; + if (!shader) { + throw new Error("Failed to create shader"); + } gl.shaderSource(shader, source); gl.compileShader(shader); return shader; }
155-166: Cache uniform locations for better performance.
gl.getUniformLocation()is called on every frame, which adds overhead. Uniform locations don't change after program linking, so they should be cached during initialization.♻️ Suggested improvement
Add private fields for cached locations:
#texelSizeLocation!: WebGLUniformLocation | null; #zoomLocation!: WebGLUniformLocation | null;Cache them in
#initialize()aftergl.useProgram(this.#program):this.#texelSizeLocation = gl.getUniformLocation(this.#program, "texelSize"); this.#zoomLocation = gl.getUniformLocation(this.#program, "zoom");Then use cached locations in the draw callback:
- const texelSizeLocation = gl.getUniformLocation( - this.#program, - "texelSize", - ); - gl.uniform2f( - texelSizeLocation, + gl.uniform2f( + this.#texelSizeLocation, 1.0 / frame.codedWidth, 1.0 / frame.codedHeight, ); - const zoomLocation = gl.getUniformLocation(this.#program, "zoom"); - gl.uniform1f(zoomLocation, this.canvas.width / frame.codedWidth); + gl.uniform1f(this.#zoomLocation, this.canvas.width / frame.codedWidth);
267-284: Cleanup could be more thorough.The texture and vertex buffer created in
#initialize()are not explicitly deleted. WhileglLoseContext()invalidates all resources, explicit deletion is cleaner and follows the eager deletion pattern used elsewhere.Consider storing and deleting the texture and vertex buffer:
// In class fields #texture!: WebGLTexture; #vertexBuffer!: WebGLBuffer; // In dispose, before glLoseContext: this.#context.deleteTexture(this.#texture); this.#context.deleteBuffer(this.#vertexBuffer);This is a minor nit since
glLoseContext()handles cleanup anyway.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (6)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (6)
61-135: Well-implemented shader pipeline with adaptive filtering.Good use of:
- Oversized triangle technique for full-canvas coverage.
- Adaptive filtering based on zoom level (tent4 for heavy downscaling, gaussian9 for moderate, direct sampling near 1:1).
failIfMajorPerformanceCaveatto avoid software rendering fallback.
181-219: LGTM!Constructor properly:
- Configures WebGL context with appropriate options for video rendering.
- References MDN best practices for
alpha: true.- Sets up context loss/restoration handlers.
221-251: LGTM!Initialization correctly sets up the WebGL program, vertex buffer, and texture with appropriate parameters for video rendering.
253-261: LGTM!Context loss handling follows the WebGL best practice:
preventDefault()signals intent to restore.- Re-initialization recreates all resources.
redraw()replays the last frame.
263-265: LGTM!Clean delegation to the
RedrawControllerfor frame management.
287-296: LGTM!Options namespace properly extends the base options with the
enableCaptureflag and includes clear documentation.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts:
- Around line 99-110: The close/abort handlers currently leave a queued frame in
#nextFrame unrendered (and potentially leaked) when #drawing is false; change
close to be async and, before closing the controller and disposing the counter,
call the instance method #draw() one final time (await it) to ensure any pending
#nextFrame is rendered (keep #captureFrame open for snapshot as before), and
mirror the same behavior in abort if you want to flush the last frame on abort;
ensure you await #draw() before calling this.#readableController.close() and
this.#counter.dispose() so the last frame is not leaked.
- Around line 152-158: The dispose() method currently only clears frames and
must also perform the same teardown as close()/abort(): call
this.#counter?.dispose() to stop the requestAnimationFrame loop and close/abort
any streams or controllers the class holds (the same streams that
close()/abort() closes) so no resources are leaked when dispose() is invoked
directly.
- Around line 93-98: The fire-and-forget call to this.#draw() can cause
unhandled rejections from #readableController.enqueue(); capture the Promise
returned by this.#draw() instead of using void and attach a .catch handler that
forwards errors to the stream controllers: call
this.#readableController.error(err) and also call the writable controller's
error handler (the same controller.error(...) used in the existing abort
handler) so enqueued-rejections are propagated and handled rather than becoming
unhandled promise rejections.
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts:
- Around line 308-325: The dispose() implementation omits disposing the
RedrawController, so call this.#controller.dispose() (or check for its existence
and call dispose) before calling super.dispose() to ensure the controller
releases its stored #lastFrame and other resources; locate the dispose() method
in the WebGL renderer and add the controller disposal
(this.#controller.dispose()) as part of the cleanup sequence.
- Around line 304-306: The override draw() in the WebGL renderer should stop
delegating to its private #controller and instead implement the rendering logic
inline so it uses the base class's RedrawController and single cloned frame
storage; remove the redundant RedrawController field (#controller) and any
#lastFrame duplication, then copy/move the WebGL-specific rendering sequence
currently around lines 182–210 into the override draw(frame: VideoFrame)
implementation (follow the BitmapVideoFrameRenderer pattern) so drawing uses the
base CanvasVideoFrameRenderer coordination and its RedrawController clone
behavior.
🧹 Nitpick comments (4)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (2)
62-81: Non-null assertions on ResizeObserver entries could fail in edge cases.The code assumes
entries[0]and the size arrays always exist with!assertions. While ResizeObserver typically provides at least one entry when called, defensive checks would be safer.♻️ Suggested defensive check
this.#resizeObserver = new ResizeObserver((entries) => { - const entry = entries[0]!; + const entry = entries[0]; + if (!entry) { + return; + } if (entry.devicePixelContentBoxSize) { - const size = entry.devicePixelContentBoxSize[0]!; + const size = entry.devicePixelContentBoxSize[0]; + if (!size) { + return; + } this.#displayWidth = size.inlineSize; this.#displayHeight = size.blockSize; } else { - const size = entry.contentBoxSize[0]!; + const size = entry.contentBoxSize[0]; + if (!size) { + return; + } this.#displayWidth = Math.round( size.inlineSize * devicePixelRatio, );
85-99:#updateSizereturn value is unused.The method returns a boolean indicating whether the size changed, but the caller in
#controller(lines 28-32) doesn't use this return value. Either remove the return value or use it to optimize (e.g., skip redraw if size unchanged but frame is same).libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (2)
13-18:createShaderdoesn't check for null return fromgl.createShader().While rare,
gl.createShader()can returnnullif the type is invalid. The non-null assertion!assumes this never happens, which is reasonable given the controlled usage, but a defensive check would be safer.♻️ Suggested defensive check
function createShader(gl: WebGLRenderingContext, type: number, source: string) { const shader = gl.createShader(type)!; + if (!shader) { + throw new Error(`Failed to create shader of type ${type}`); + } gl.shaderSource(shader, source); gl.compileShader(shader); return shader; }
196-207:getUniformLocationis called on every draw, which is inefficient.
gl.getUniformLocation()is called inside the draw callback for bothtexelSizeandzoomuniforms. These locations are stable once the program is linked and should be cached during initialization for better performance.♻️ Suggested optimization
#context: WebGLRenderingContext; #program!: WebGLProgram; +#texelSizeLocation!: WebGLUniformLocation | null; +#zoomLocation!: WebGLUniformLocation | null; // In #initialize(): this.#program = createProgram(...); gl.useProgram(this.#program); +this.#texelSizeLocation = gl.getUniformLocation(this.#program, "texelSize"); +this.#zoomLocation = gl.getUniformLocation(this.#program, "zoom"); // In RedrawController callback: -const texelSizeLocation = gl.getUniformLocation(this.#program, "texelSize"); -gl.uniform2f(texelSizeLocation, ...); +gl.uniform2f(this.#texelSizeLocation, ...); -const zoomLocation = gl.getUniformLocation(this.#program, "zoom"); -gl.uniform1f(zoomLocation, ...); +gl.uniform1f(this.#zoomLocation, ...);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libraries/adb-credential-web/src/storage/password.tslibraries/adb-credential-web/src/storage/prf/storage.tslibraries/media-codec/src/index.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-14T11:20:12.044Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 780
File: libraries/adb-credential-web/src/storage/type.ts:1-7
Timestamp: 2025-08-14T11:20:12.044Z
Learning: The TangoDataStorage interface is only defined in libraries/adb-credential-web/src/storage/type.ts. There is no duplicate definition in libraries/adb/src/storage/type.ts as that file does not exist.
Applied to files:
libraries/adb-credential-web/src/storage/password.tslibraries/adb-credential-web/src/storage/prf/storage.ts
📚 Learning: 2025-08-14T11:20:12.044Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 780
File: libraries/adb-credential-web/src/storage/type.ts:1-7
Timestamp: 2025-08-14T11:20:12.044Z
Learning: The TangoDataStorage interface is defined in libraries/adb-credential-web/src/storage/type.ts and there is no duplicate definition in libraries/adb/src/storage/type.ts (which doesn't exist).
Applied to files:
libraries/adb-credential-web/src/storage/password.tslibraries/adb-credential-web/src/storage/prf/storage.ts
📚 Learning: 2025-08-14T11:06:21.851Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 780
File: libraries/adb-credential-web/src/storage/index.ts:1-5
Timestamp: 2025-08-14T11:06:21.851Z
Learning: The yume-chan/adb-credential-web package doesn't use "exports" in package.json for better compatibility. Instead, they re-export everything from the main entrypoint rather than using subpath exports.
Applied to files:
libraries/adb-credential-web/src/storage/password.tslibraries/adb-credential-web/src/storage/prf/storage.ts
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts
📚 Learning: 2025-08-14T11:20:01.711Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 780
File: libraries/adb-credential-web/src/storage/prf/source.ts:2-10
Timestamp: 2025-08-14T11:20:01.711Z
Learning: TypeScript 5.9 introduced generic TypedArrays (e.g., Uint8Array<ArrayBuffer>, Int32Array<SharedArrayBuffer>) to preserve the distinction between ArrayBuffer and SharedArrayBuffer at the type level. The syntax Uint8Array<ArrayBuffer> is valid and recommended in TypeScript 5.9+ for type safety. This change helps prevent unsafe mixing of ArrayBuffer and SharedArrayBuffer types, especially important for WebCrypto APIs and other browser APIs that have different behavior for these buffer types.
Applied to files:
libraries/adb-credential-web/src/storage/prf/storage.ts
📚 Learning: 2025-09-04T14:26:19.473Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 797
File: libraries/adb/src/features.ts:1-7
Timestamp: 2025-09-04T14:26:19.473Z
Learning: The user intentionally uses TypeScript declaration merging to combine AdbFeature value and type into an enum-like object pattern, and prefers to disable Biome linting rather than rename imports.
Applied to files:
libraries/adb-credential-web/src/storage/prf/storage.ts
🧬 Code graph analysis (4)
libraries/adb-credential-web/src/storage/password.ts (1)
libraries/adb/src/utils/array-buffer.ts (1)
toLocalUint8Array(1-8)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (3)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
VideoFrameRenderer(4-8)libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (1)
RedrawController(6-99)libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (1)
createCanvas(1-9)
libraries/adb-credential-web/src/storage/prf/storage.ts (1)
libraries/adb/src/utils/array-buffer.ts (1)
toLocalUint8Array(1-8)
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (4)
libraries/scrcpy-decoder-webcodecs/src/video/pool.ts (1)
Pool(1-37)libraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts (1)
VideoFrameCapturer(1-42)libraries/stream-extra/src/push-readable.ts (2)
PushReadableStreamController(6-14)PushReadableStream(41-307)libraries/scrcpy-decoder-tinyh264/src/utils/performance.ts (1)
PerformanceCounter(3-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (13)
libraries/media-codec/src/index.ts (1)
2-2: LGTM!The new
formatmodule export is correctly added, follows the existingexport *pattern used forav1andnalu, and maintains alphabetical ordering.libraries/adb-credential-web/src/storage/password.ts (2)
2-2: LGTM!The import addition is necessary for the conversion used on line 96.
83-97: Approve the type relaxation and conversion.The change from
Uint8Array<ArrayBuffer>toUint8Arrayimproves API flexibility, andtoLocalUint8Arraycorrectly ensures the buffer is suitable for the Web Crypto API.One minor note: if
toLocalUint8Arraycreates a copy (when the buffer isn't anArrayBuffer), that copy won't be explicitly cleared. However, this is consistent with the existing acknowledged limitations (lines 107-110) and the copy becomes eligible for GC immediately afterencryptcompletes.libraries/adb-credential-web/src/storage/prf/storage.ts (2)
1-1: LGTM!Both imports are correctly added:
toLocalUint8Arrayis used at line 125 for the encryption call, andMaybeErroris used for theload()generator return type at line 140.
87-126: LGTM!The type relaxation from
Uint8Array<ArrayBuffer>toUint8Arraypaired with thetoLocalUint8Array()call is the right approach—it makes the API more flexible for callers while ensuring WebCrypto compatibility internally. Based on learnings, this aligns with TypeScript 5.9+ generic TypedArray patterns for proper ArrayBuffer/SharedArrayBuffer distinction.libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (2)
35-42: WritableStream error propagation is missing.The
WritableStreamwirescloseandaborttodispose(), but there's no error handling for the underlying#controller.draw(frame)call in thewritecallback. Ifdraw()rejects, the error will propagate to the stream, but there's no explicit handling. This is likely intentional since the stream will error naturally, but worth noting.Additionally, the
dispose()method doesn't return anything explicitly, yetcloseandabortcallbacks expectundefinedor a promise. Sincedispose()is typed to returnundefined, this works correctly.
108-116:dispose()doesn't dispose the#controllerbefore other cleanup.The
RedrawController.dispose()is called last, but#controllercould still be processing a frame whendispose()is invoked. This ordering is fine sinceRedrawController.dispose()sets an error state that prevents further draws, but consider whether the#writablestream should also be explicitly aborted or closed to prevent further writes.Based on learnings,
controller.error()in TransformStream clears cancel callbacks, so explicit cleanup is important. While this isn't a TransformStream, the pattern applies: ensure no dangling operations remain after dispose.libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (3)
13-15: Module-level singleton pool may cause issues with multiple RendererController instances.
VideoFrameCapturerPoolis initialized once at module level with capacity 4. If multipleRendererControllerinstances callsnapshot()concurrently, they share this pool. This is likely intentional for resource efficiency, but if the pool becomes exhausted,borrow()will block until a capturer is returned.
114-136: Race condition:#nextFramecould be set between while-check and undefined assignment.In
#draw(), between line 128 checkingthis.#nextFrameand line 129 setting it toundefined, thewritecallback could set a new#nextFrame. However, since JavaScript is single-threaded andawaitis the only yield point (line 130), this race cannot actually occur—the write callback would be blocked by the ongoing await.The logic is correct. The while loop ensures all frames are processed even if new ones arrive during the await.
138-150:snapshot()doesn't handle closed/disposed state.If
snapshot()is called afterdispose(),#captureFramewill beundefined(asdispose()sets it toundefined), so it will returnundefined. However, if called during a race wheredispose()is running concurrently, the frame could be closed whilecapture()is using it.Since
close()andabort()intentionally don't close#captureFrame, this allows post-close snapshots. Butdispose()closes it, so callers should be aware thatdispose()invalidates snapshot capability.libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (3)
53-58: Shader deletion infinallyblock follows WebGL best practices.Deleting shaders after linking is correct—once attached and linked, the program holds references, and explicit deletion marks them for garbage collection when the program is deleted. This follows MDN's "delete objects eagerly" guidance.
181-213: RedrawController callback correctly handles context loss.The early return on
gl.isContextLost()prevents WebGL calls on a lost context, which would otherwise throw errors or be no-ops. The#handleContextRestoredcallback re-initializes and triggers a redraw, which is the correct pattern.
294-302: Context restoration doesn't delete the old program before re-initialization.When
#handleContextRestoredcalls#initialize(), a new program is created. The old#programreference becomes stale (the context was lost, so the old program is already invalid), but assigning over it is fine. This is correct behavior since context loss invalidates all WebGL objects.
| override dispose(): undefined { | ||
| this.#context.deleteProgram(this.#program); | ||
|
|
||
| this.canvas.removeEventListener( | ||
| "webglcontextlost", | ||
| this.#handleContextLost, | ||
| ); | ||
| this.canvas.removeEventListener( | ||
| "webglcontextrestored", | ||
| this.#handleContextRestored, | ||
| ); | ||
|
|
||
| // Lose contexts eagerly | ||
| // https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#lose_contexts_eagerly | ||
| glLoseContext(this.#context); | ||
| return undefined; | ||
|
|
||
| super.dispose(); | ||
| } |
There was a problem hiding this comment.
dispose() cleans up properly and calls super.dispose().
The disposal sequence is correct:
- Delete the WebGL program
- Remove event listeners
- Lose the WebGL context (following MDN best practices)
- Call
super.dispose()for base class cleanup
However, #controller.dispose() is not called. The WebGL renderer's RedrawController should also be disposed to close its stored #lastFrame.
🐛 Suggested fix
override dispose(): undefined {
this.#context.deleteProgram(this.#program);
this.canvas.removeEventListener(
"webglcontextlost",
this.#handleContextLost,
);
this.canvas.removeEventListener(
"webglcontextrestored",
this.#handleContextRestored,
);
+ this.#controller.dispose();
+
// Lose contexts eagerly
// https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#lose_contexts_eagerly
glLoseContext(this.#context);
super.dispose();
}🤖 Prompt for AI Agents
In @libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts around lines
308 - 325, The dispose() implementation omits disposing the RedrawController, so
call this.#controller.dispose() (or check for its existence and call dispose)
before calling super.dispose() to ensure the controller releases its stored
#lastFrame and other resources; locate the dispose() method in the WebGL
renderer and add the controller disposal (this.#controller.dispose()) as part of
the cleanup sequence.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libraries/stream-extra/src/push-readable.ts`:
- Around line 261-270: The catch currently replaces the original rejection with
new Error("") losing error info; update the else branch that sets ready =
promise.catch(...) so it preserves and propagates the original error (e.g.,
ready = promise.catch(err => { throw err; }) or simply remove the catch and set
ready = promise) — locate the ready variable assignment inside the enqueue logic
that wraps enqueue(chunk) and change the catch to re-throw the caught error or
drop the catch entirely so rejection reasons are not discarded.
🧹 Nitpick comments (2)
libraries/scrcpy/src/2_0/impl/parse-audio-stream-metadata.ts (1)
84-93: Good modernization tofor await...ofiteration.The switch to async iteration is cleaner and more idiomatic than a manual reader loop.
Minor nit:
streamon line 88 shadows the function parameter on line 13. Consider renaming toinnerStreamorreleasedStreamfor clarity.Optional: Rename to avoid shadowing
- const stream = buffered.release(); + const releasedStream = buffered.release(); - for await (const chunk of stream) { + for await (const chunk of releasedStream) { await controller.enqueue(chunk); }libraries/stream-extra/src/push-readable.spec.ts (1)
690-752: Good test coverage for pending enqueue handling.This test validates that when the source Promise resolves (triggering implicit close), any pending
enqueuecalls are properly woken up and ignored. The test name "should cancel pendingenqueue" could be slightly misleading since it tests implicit close behavior rather than consumer cancel—consider renaming to "should ignore pendingenqueueon implicit close" for clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libraries/adb/src/daemon/dispatcher.tslibraries/android-bin/src/bug-report.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.tslibraries/scrcpy/src/2_0/impl/parse-audio-stream-metadata.tslibraries/stream-extra/src/push-readable.spec.tslibraries/stream-extra/src/push-readable.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-12T06:58:27.724Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:27.724Z
Learning: In TransformStream implementations, calling `controller.error()` immediately clears the internal `[[flushAlgorithm]]` and `[[cancelAlgorithm]]` slots, which prevents the `flush()` and `cancel()` callbacks from being invoked. This means that if a decoder's error callback calls `controller.error()`, the stream's `cancel()` callback will not subsequently run, eliminating concerns about double-close scenarios.
Applied to files:
libraries/android-bin/src/bug-report.tslibraries/stream-extra/src/push-readable.tslibraries/stream-extra/src/push-readable.spec.ts
🧬 Code graph analysis (4)
libraries/adb/src/daemon/dispatcher.ts (2)
libraries/adb-scrcpy/src/client.ts (1)
controller(285-287)libraries/adb/src/daemon/packet.ts (2)
AdbCommand(5-12)AdbCommand(14-14)
libraries/stream-extra/src/push-readable.ts (4)
libraries/stream-extra/src/stream.ts (1)
AbortController(13-23)libraries/stream-extra/src/duplex.ts (1)
close(115-131)libraries/stream-extra/src/pipe-from.ts (1)
close(23-26)libraries/stream-extra/src/consumable.ts (1)
error(40-42)
libraries/scrcpy/src/2_0/impl/parse-audio-stream-metadata.ts (2)
libraries/adb-scrcpy/src/video.ts (1)
stream(22-24)libraries/adb-scrcpy/src/client.ts (1)
controller(285-287)
libraries/stream-extra/src/push-readable.spec.ts (1)
libraries/stream-extra/src/push-readable.ts (2)
PushReadableLogger(28-47)PushReadableStream(49-332)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (20.x)
🔇 Additional comments (12)
libraries/android-bin/src/bug-report.ts (1)
261-264: LGTM! Abort signal propagation enables proper cancellation.Passing
controller.abortSignaltospawnallows thebugreportzprocess to be aborted when the consumer cancels the stream. This aligns well with the PR's streaming-based control approach.libraries/stream-extra/src/push-readable.spec.ts (2)
37-93: LGTM! Test assertions correctly updated for new logger signature.The mapping
log.mock.calls.map((call) => call.arguments[0])correctly extracts the single event object passed to the logger, aligning with the updatedPushReadableLogger<T>type.
432-527: Good edge case coverage for concurrent enqueue calls.This test properly validates that multiple non-awaited
enqueuecalls withhighWaterMark: 0are handled gracefully, even though this pattern isn't recommended for production use. The step-by-step verification withresetCalls()makes the test clear and maintainable.libraries/stream-extra/src/push-readable.ts (7)
14-22: API change:enqueuenow returnsPromise<boolean>.The return type change from
Promise<void>toPromise<boolean>is a breaking change. The boolean indicates whether the chunk was successfully enqueued (true) or ignored due to cancellation/close (false). This is a useful improvement for producers to know if their data was accepted.
74-185: LGTM! Well-structured enqueue implementation with proper backpressure handling.The enqueue logic correctly handles multiple cases:
- Aborted streams return
falsewithout throwing- Error state delegates to controller to throw
- Zero highWaterMark special case allows immediate enqueue when consumer is ready
- Backpressure waits on
waterMarkLowpromise with proper resolve/reject handling
187-219: LGTM! Close function correctly handles explicit vs implicit close.The conditional
abortController.signal.aborted || (stopped && !explicit)properly allows explicit close on cancelled streams while preventing duplicate implicit closes. ThewaterMarkLow?.reject()correctly wakes pending enqueues.
221-242: LGTM! Error function is straightforward and correct.Per the comment and retrieved learnings,
controller_.error()won't throw on already-closed/errored/cancelled streams, so no conditional checks are needed. Based on learnings, callingcontroller.error()clears internal flush/cancel slots, which is the expected behavior.
279-289: LGTM! Source promise handling is correct.The
!stoppedcheck before attaching promise handlers prevents redundant close/error calls if the producer already explicitly closed or errored the stream. Usingclose(false)anderror(e, false)correctly marks these as implicit operations.
291-310: LGTM! Pull callback properly manages backpressure signals.The pull callback correctly resolves the
waterMarkLowpromise to wake pending enqueues, and clears it to prevent multiple resolves. ThezeroHighWaterMarkAllowEnqueueflag properly handles the special case wherehighWaterMark: 0requires a different signaling mechanism.
311-327: LGTM! Cancel callback correctly coordinates cleanup.The cancel callback properly sets
stopped, aborts the controller with the cancellation reason, and rejectswaterMarkLowto wake any pending enqueue operations. The order of operations ensures consistent state when the pending enqueue wakes up.libraries/adb/src/daemon/dispatcher.ts (2)
148-156: LGTM - Fire-and-forget pattern for OPEN is safe.The socket is only registered in
#socketsafter the handler completes and OKAY is sent (lines 350-355), so the device won't send WRITE/CLOSE packets until this completes. The error propagation viacontroller.error(e)correctly terminates the connection on fatal errors.
157-165: Fire-and-forget WRITE handling is safe; concurrent enqueues are serialized.The concern about concurrent WRITE packets causing out-of-order data is mitigated by the
PushReadableStreamControllerimplementation. Itsenqueue()method internally serializes concurrent calls through areadypromise chain (seepush-readable.tslines 251–268): each subsequent call awaits the previous one, ensuring FIFO ordering regardless of when#handleWrite()tasks complete.No action needed.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts`:
- Around line 238-247: The dispose() method must explicitly close the
VideoDecoder stream to avoid leaking hardware resources: ensure you force-close
or abort this.#rawDecoder's writable stream before disposing other helpers so
the VideoDecoderStream flush/cancel callbacks run; call
this.#rawDecoder.writable.getWriter().close() (or
this.#rawDecoder.writable.getWriter().abort() on error) and await it (or handle
promise) so the underlying VideoDecoder is closed, then proceed to call
this.#size.dispose(), this.#pause.dispose(), this.#renderController.dispose(),
and this.#renderer.dispose?.().
♻️ Duplicate comments (5)
libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (2)
100-111: Resource leak:#nextFramenot closed onclose()andabort().The comments state "Don't close
#nextFrameto make sure all frames are rendered," but afterclose()orabort(), no more frames will be rendered. If#nextFrameholds aVideoFramewhen the stream terminates, it will leak.🔧 Suggested fix
close: () => { + this.#nextFrame?.close(); + this.#nextFrame = undefined; this.#readableController.close(); this.#counter.dispose(); // Don't close `#captureFrame` to allow using `snapshot` on the last frame - // Don't close `#nextFrame` to make sure all frames are rendered }, abort: (reason) => { + this.#nextFrame?.close(); + this.#nextFrame = undefined; this.#readableController.error(reason); this.#counter.dispose(); // Don't close `#captureFrame` to allow using `snapshot` on the last frame - // Don't close `#nextFrame` to make sure all frames are rendered },
142-154:dispose()doesn't dispose#counter, potentially leaking therequestAnimationFrameloop.The
#counteris disposed inclose()andabort()callbacks, but ifdispose()is called directly without going through stream close/abort, the counter won't be disposed.🔧 Suggested fix
dispose() { this.#captureFrame?.close(); this.#captureFrame = undefined; this.#nextFrame?.close(); this.#nextFrame = undefined; + this.#counter.dispose(); + this.#readableController.close(); // Throw a similar error to native TransformStream this.#writableController.error( new TypeError("The transform stream has been terminated"), ); }libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (2)
12-17: Missing null check forgl.createShader().
gl.createShader()can returnnullif the context is lost or the type is invalid. The non-null assertion masks this, causing subsequent calls to fail with cryptic errors.🔧 Suggested fix
function createShader(gl: WebGLRenderingContext, type: number, source: string) { - const shader = gl.createShader(type)!; + const shader = gl.createShader(type); + if (!shader) { + throw new Error("Failed to create shader"); + } gl.shaderSource(shader, source); gl.compileShader(shader); return shader; }
31-34: Missing null check forgl.createProgram().
gl.createProgram()can returnnullif the context is lost. Callinggl.attachShader(null, ...)will silently fail.🔧 Suggested fix
const program = gl.createProgram(); + if (!program) { + gl.deleteShader(vertexShader); + gl.deleteShader(fragmentShader); + throw new Error("Failed to create program"); + } gl.attachShader(program, vertexShader);libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (1)
174-199: Critical: Pipeline errors are silently swallowed.The
.catch(noop)at line 199 suppresses all errors from the decoding/rendering pipeline. Based on learnings, configuration failures and decoder errors should propagate to allow the client to handle them appropriately. Additionally, sincecontroller.error()in TransformStreams clears the cancel algorithm, error handling paths cannot rely oncancel()for cleanup—making silent error swallowing even more problematic as cleanup may be incomplete.Consider exposing errors via a callback in Options or at minimum logging them:
🔧 Proposed fix
export namespace WebCodecsVideoDecoder { export interface Options extends Pick< VideoDecoderConfig, "hardwareAcceleration" | "optimizeForLatency" > { /** * The video codec to decode */ codec: ScrcpyVideoCodecId; renderer?: VideoFrameRenderer | undefined; + + /** + * Called when the pipeline encounters an error. + * If not provided, errors are logged to console. + */ + onError?: (error: unknown) => void; } }In constructor:
+ const handleError = options.onError ?? ((error: unknown) => { + console.error("Video pipeline error:", error); + }); + void this.#pause.readable // ... pipeline stages ... .pipeTo(renderer.writable) - // Errors will be handled by source stream - .catch(noop); + .catch(handleError);
🧹 Nitpick comments (3)
libraries/stream-extra/src/push-readable.ts (1)
210-211: Consider clearingwaterMarkLowafter rejection for consistency.In
pull(line 296),waterMarkLowis cleared after resolution. For consistency, you could also clear it after rejection inclose,error, andcancel. This isn't functionally necessary sincestopped=trueprevents new waiters, but it would make the lifecycle slightly cleaner.♻️ Suggested consistency improvement
stopped = true; controller_.close(); // Wake up pending `enqueue` waterMarkLow?.reject(); + waterMarkLow = undefined;(Similar change in
errorat line 234 andcancelat line 316)libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (1)
79-94: Inconsistent error handling betweendraw()andredraw().In
draw()(lines 47-49), errors are captured and re-thrown. Inredraw()(line 90), errors are captured but silently swallowed. This means:
draw()failures propagate to callersredraw()failures are only stored for the next operationIf this is intentional (redraw failures shouldn't abort the caller since
redraw()returnsvoid), consider adding a brief comment explaining the design choice.libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
69-84: Non-null assertions on ResizeObserver entry arrays.Lines 70, 73, and 77 use non-null assertions (
!) on array access. WhileResizeObservertypically provides at least one entry per observed element, thedevicePixelContentBoxSizeandcontentBoxSizearrays could theoretically be empty in edge cases.Consider defensive checks or document why these are safe:
const entry = entries[0]; if (!entry) return; const size = entry.devicePixelContentBoxSize?.[0] ?? entry.contentBoxSize?.[0]; if (!size) return;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.tslibraries/scrcpy-decoder-webcodecs/src/video/render/redraw.tslibraries/scrcpy-decoder-webcodecs/src/video/render/type.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.tslibraries/scrcpy-decoder-webcodecs/src/video/snapshot.tslibraries/scrcpy-decoder-webcodecs/src/video/utils/snapshot.tslibraries/stream-extra/src/push-readable.ts
💤 Files with no reviewable changes (1)
- libraries/scrcpy-decoder-webcodecs/src/video/snapshot.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libraries/scrcpy-decoder-webcodecs/src/video/render/bitmap.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T06:58:19.519Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:19.519Z
Learning: In TransformStream implementations, calling controller.error() immediately clears the internal [[flushAlgorithm]] and [[cancelAlgorithm]] slots, which prevents the flush() and cancel() callbacks from being invoked. Therefore, if a decoder’s error callback calls controller.error(), the stream’s cancel() callback will not run later. Treat this as a सच rule: do not rely on error() to trigger cleanup through cancel; instead, perform explicit cleanup and, if needed, initiate cancel() or reject the downstream promise yourself. Apply this guidance broadly to all TransformStream-based code paths (not just this file) and ensure error handling paths explicitly manage resource cleanup and stream termination without assuming cancel will be invoked.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/utils/snapshot.tslibraries/scrcpy-decoder-webcodecs/src/video/render/canvas.tslibraries/scrcpy-decoder-webcodecs/src/video/render/redraw.tslibraries/scrcpy-decoder-webcodecs/src/video/render/type.tslibraries/scrcpy-decoder-webcodecs/src/video/render/webgl.tslibraries/scrcpy-decoder-webcodecs/src/video/decoder.tslibraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts
📚 Learning: 2026-01-12T06:58:27.724Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 827
File: libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts:105-120
Timestamp: 2026-01-12T06:58:27.724Z
Learning: In TransformStream implementations, calling `controller.error()` immediately clears the internal `[[flushAlgorithm]]` and `[[cancelAlgorithm]]` slots, which prevents the `flush()` and `cancel()` callbacks from being invoked. This means that if a decoder's error callback calls `controller.error()`, the stream's `cancel()` callback will not subsequently run, eliminating concerns about double-close scenarios.
Applied to files:
libraries/stream-extra/src/push-readable.ts
📚 Learning: 2025-08-24T03:50:43.091Z
Learnt from: yume-chan
Repo: yume-chan/ya-webadb PR: 782
File: libraries/scrcpy-decoder-tinyh264/src/decoder.ts:193-196
Timestamp: 2025-08-24T03:50:43.091Z
Learning: In TinyH264Decoder, configuration failures should propagate the actual error up the client pipeline rather than being recovered locally. The decoder is designed to handle mid-stream configuration switches when the video encoder restarts, and subsequent valid config packets will re-configure correctly regardless of current decoder state.
Applied to files:
libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
🧬 Code graph analysis (4)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (4)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
VideoFrameRenderer(4-10)libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (2)
RedrawController(6-102)draw(32-56)libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (1)
createCanvas(1-9)libraries/scrcpy-decoder-webcodecs/src/video/utils/snapshot.ts (1)
canvasToBlob(1-17)
libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (3)
libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (1)
frame(92-106)libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (1)
frame(115-140)libraries/stream-extra/src/stream.ts (1)
AbortController(13-23)
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
libraries/stream-extra/src/consumable.ts (1)
WritableStream(71-71)
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (1)
libraries/scrcpy-decoder-tinyh264/src/utils/gl.ts (2)
glCreateContext(11-59)glLoseContext(61-67)
🔇 Additional comments (25)
libraries/scrcpy-decoder-webcodecs/src/video/utils/snapshot.ts (1)
1-17: LGTM!Clean utility function with proper handling for both canvas types. The Promise wrapper for
HTMLCanvasElement.toBlobcorrectly handles the null case (which can occur when canvas is tainted or conversion fails), and the type narrowing viainstanceof OffscreenCanvasis idiomatic.libraries/stream-extra/src/push-readable.ts (7)
14-22: LGTM! Clean interface update for backpressure signaling.The
enqueuereturn type change fromPromise<void>toPromise<boolean>is a well-designed API improvement. Returningfalsewhen enqueue is ignored (due to abort) allows producers to react appropriately without needing exception handling for expected flow control scenarios.
62-72: LGTM! Clean state management setup.The separation between
stopped(producer-initiated) andabortController.signal.aborted(consumer-initiated) is well-designed. Thereadypromise chain for serializing enqueue calls andwaterMarkLowfor backpressure signaling are appropriate primitives.
74-185: LGTM! Well-structured backpressure handling.The enqueue logic correctly handles:
- Aborted state with graceful
falsereturn- Error state detection via
desiredSize === null- Special
zeroHighWaterMarkcase- Backpressure waiting with proper rejection handling
The detailed comments explaining each case are valuable for maintainability.
187-219: LGTM! Proper lifecycle management.The close logic correctly:
- Differentiates explicit (producer-called) vs implicit (promise-resolved) close
- Wakes pending enqueue via
waterMarkLow?.reject()- Handles already-stopped states gracefully
221-242: LGTM! Robust error handling.The error function correctly sets terminal state unconditionally. The comment about
controller.errornot throwing on closed/errored/cancelled streams is helpful documentation.
251-265: LGTM! Clever optimization for synchronous enqueues.The logic correctly avoids unnecessary promise chaining when enqueues complete synchronously. The
asynckeyword ensures the interface contract (Promise<boolean>) is satisfied while the internal optimization prevents extra microtasks.
287-323: LGTM! Correct consumer-side handling.The
pullcallback properly signals when the consumer is ready to receive data, handling both the normal backpressure case (waterMarkLowresolution) and the zerohighWaterMarkedge case. Thecancelcallback correctly terminates all pending operations.libraries/scrcpy-decoder-webcodecs/src/video/render/redraw.ts (1)
1-20: LGTM on the overall design.The
RedrawControllereffectively coordinates drawing and redrawing with proper frame lifecycle management, promise chaining for sequential operations, and cancellation support viaAbortController.libraries/scrcpy-decoder-webcodecs/src/video/render/flow-control.ts (1)
65-113: Well-structured stream coordination with frame-skipping logic.The implementation correctly:
- Clones frames for capture vs rendering (
#captureFrameand#nextFrameare separate)- Skips intermediate frames when the renderer can't keep up (lines 88-92)
- Propagates abort signals from readable to writable (lines 74-77)
- Uses a non-blocking draw pattern to avoid backpressure issues
libraries/scrcpy-decoder-webcodecs/src/video/render/type.ts (1)
4-10: LGTM - clean stream-based interface.The
VideoFrameRendererinterface is well-designed:
writableproperty uses correct spelling (consistent with standard stream APIs)- Optional
snapshot()anddispose()methods allow flexible implementations- Return types are appropriate (
undefinedfor dispose aligns with the codebase pattern)libraries/scrcpy-decoder-webcodecs/src/video/render/canvas.ts (3)
40-47: Verify WritableStream lifecycle whendispose()is called externally.Both
closeandabortcallbacks calldispose(), which is correct. However, ifdispose()is called directly (not through stream termination), theWritableStreammay remain in an inconsistent state since it won't be explicitly closed or errored.Consider whether
dispose()should also terminate the writable stream, or document that callers should close the stream before callingdispose().
49-90: Well-structured constructor with proper separation of concerns.The constructor correctly:
- Accepts a draw callback, optional canvas, and options
- Validates
canvasSize: "display"only works withHTMLCanvasElement- Sets up
ResizeObserveronly when needed- Triggers redraws on resize to update canvas content
138-154: Clear and helpful JSDoc forcanvasSizeoption.The documentation clearly explains the three modes and their requirements. This helps users understand when to use each option.
libraries/scrcpy-decoder-webcodecs/src/video/render/webgl.ts (4)
60-162: Comprehensive shader implementation with adaptive filtering.The fragment shader intelligently selects filtering based on zoom level:
- Direct sampling at near 1:1 (zoom > 0.95)
- Bicubic Mitchell-Netravali at moderate downscaling (zoom > 0.5)
- Tent filter for heavy downscaling
This provides good quality across different display scenarios.
193-232: Clean integration with base class via draw callback.The rendering logic is now correctly passed as a callback to the base
CanvasVideoFrameRenderer, eliminating the previous issue of duplicateRedrawControllerinstances. This is a significant architectural improvement.
306-314: Context loss handling follows best practices.The implementation correctly:
- Calls
e.preventDefault()on context lost to signal intent to restore- Reinitializes WebGL state on context restored
- Triggers redraw to restore the display
Note: If
redraw()is called whenlastFramehas been closed (per the retrieved learning about frame lifecycle), it will gracefully return early due to the!this.#lastFramecheck inRedrawController.redraw().
323-340: Proper cleanup sequence indispose().The disposal correctly:
- Deletes the WebGL program
- Removes event listeners (preventing memory leaks)
- Loses the WebGL context eagerly (per MDN best practices)
- Calls
super.dispose()for base class cleanupThe previous issue about disposing a separate
#controllerno longer applies since the class now uses the base class's controller.libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts (7)
1-21: LGTM!The imports are well-organized and align with the new streaming pipeline architecture. The separation of codec transforms, rendering components, and utility streams reflects good modularity.
23-47: LGTM!Static capability checks and parameter encapsulation are correctly implemented.
49-104: LGTM!Good encapsulation of streaming components. The public getters cleanly expose decoder metrics (queue size, frame counts) without leaking internal implementation details.
106-144: LGTM!Good documentation on the
framesPresentedgetter explaining the vsync behavior and Chrome bug. The metrics exposure provides comprehensive observability for decode/render performance.
202-212: LGTM!Clean delegation to the pause controller. The visibility tracking integration is a nice UX improvement for background tab handling.
214-236: LGTM!The snapshot implementation correctly prioritizes the native renderer's snapshot capability and falls back to
BitmapVideoFrameRendererwhen unavailable. The try/finally ensures the fallback renderer is properly disposed.
250-261: LGTM!Clean interface definition that properly leverages
VideoDecoderConfigtypes for hardware acceleration and latency optimization settings.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| dispose() { | ||
| this.#captureFrame?.close(); | ||
| // Most cleanup happens automatically when `writable` ends | ||
| // (in each stream's `close` callback). | ||
| // This method cleanup things that still available after `writable` ends | ||
|
|
||
| this.#counter.dispose(); | ||
| this.#renderer.dispose(); | ||
| this.#size.dispose(); | ||
| this.#nextFrame?.close(); | ||
|
|
||
| if (this.#rawDecoder.state !== "closed") { | ||
| this.#rawDecoder.close(); | ||
| } | ||
|
|
||
| // This class doesn't need to guard against multiple dispose calls | ||
| // since most of the logic is already handled in `#pause` | ||
| this.#pause.dispose(); | ||
|
|
||
| this.#setError(new Error("Attempt to write to a disposed decoder")); | ||
| this.#renderController.dispose(); | ||
| this.#renderer.dispose?.(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check VideoDecoderStream class and its cleanup handling
rg -n "class VideoDecoderStream" libraries/scrcpy-decoder-webcodecs/src/ -A 30
# Check how `#rawDecoder` is used and initialized
rg -n "#rawDecoder" libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts -B 2 -A 2
# Check if dispose is called on streams in decoder.ts
rg -n "writable\.|readable\.|\.abort\|\.close" libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts -B 1 -A 1
# Check VideoDecoder close/stop methods
rg -n "VideoDecoder" libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts -B 2 -A 5Repository: yume-chan/ya-webadb
Length of output: 5836
🏁 Script executed:
#!/bin/bash
# Get the flush and cancel callbacks in VideoDecoderStream
rg -n "flush:|cancel:" libraries/scrcpy-decoder-webcodecs/src/video/utils/video-decoder-stream.ts -A 10
# Check the full dispose() method in decoder.ts
sed -n '238,250p' libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
# Check if there's any writable stream handling before dispose
rg -n "this.#writable\|writable.abort\|writable.close" libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts -B 2 -A 2
# Check `#timestampTransforms` initialization and type
rg -n "#timestampTransforms" libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts -B 1 -A 1Repository: yume-chan/ya-webadb
Length of output: 1950
🏁 Script executed:
#!/bin/bash
# Check writable stream creation and initialization
rg -n "WritableStream\|writable\s*=" libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts -B 2 -A 2
# Check full dispose and constructor area to understand stream lifecycle
sed -n '50,120p' libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
# Check if `#writable` is a field
rg -n "^\s*#writable" libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts -A 1
# Look for any abort or close calls in the entire file
rg -n "abort\|\.close()" libraries/scrcpy-decoder-webcodecs/src/video/decoder.tsRepository: yume-chan/ya-webadb
Length of output: 1698
🏁 Script executed:
#!/bin/bash
# Get the constructor
sed -n '130,210p' libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts
# Check what PauseController does
rg -n "class PauseController" libraries/scrcpy-decoder-webcodecs/src/ -A 30Repository: yume-chan/ya-webadb
Length of output: 2940
🏁 Script executed:
#!/bin/bash
# Find and examine PauseController
fd "pause" libraries/scrcpy-decoder-webcodecs/src/ -type f
# Search for PauseController class
rg -n "class PauseController" libraries/scrcpy-decoder-webcodecs/src/ -A 50Repository: yume-chan/ya-webadb
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Find PauseController
rg -n "class PauseController" libraries/scrcpy-decoder-webcodecs/src/ -A 50
# Also search for imports to locate it
rg -n "PauseController" libraries/scrcpy-decoder-webcodecs/src/video/decoder.tsRepository: yume-chan/ya-webadb
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Check if there are tests for dispose method
fd -e test.ts -e spec.ts libraries/scrcpy-decoder-webcodecs/
# Check for any documentation or comments about dispose lifecycle
rg -n "dispose\|abort\|close" libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts -B 2 -A 2
# Check package.json for dependencies
cat libraries/scrcpy-decoder-webcodecs/package.json | grep -A 20 "dependencies"
# See how dispose is used in tests or examples
rg -n "\.dispose()" libraries/scrcpy-decoder-webcodecs/ -B 3 -A 3Repository: yume-chan/ya-webadb
Length of output: 7382
🏁 Script executed:
#!/bin/bash
# Search for PauseController usage/examples
rg -n "PauseController" libraries/ -B 3 -A 3
# Check if there are integration tests
fd -e test.ts -e spec.ts libraries/scrcpy-decoder-webcodecs/
# Look for any lifecycle/cleanup tests
rg -n "dispose\|cleanup\|teardown" libraries/scrcpy-decoder-webcodecs/src/ -B 2 -A 2 | head -50
# Check the actual export/interface of WebCodecsVideoDecoder
rg -n "export.*WebCodecsVideoDecoder\|export class\|export interface" libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts -A 5Repository: yume-chan/ya-webadb
Length of output: 7344
🏁 Script executed:
#!/bin/bash
# Get the PauseController.dispose() method
sed -n '1,223p' libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts | grep -n "dispose" -A 15
# Full view of dispose method
rg -n "dispose\(" libraries/scrcpy-decoder-tinyh264/src/utils/pause.ts -A 20Repository: yume-chan/ya-webadb
Length of output: 5657
🏁 Script executed:
#!/bin/bash
# Check TimestampTransforms class and if it needs dispose
rg -n "class TimestampTransforms" libraries/scrcpy-decoder-webcodecs/src/ -A 50
# Check if TimestampTransforms has dispose method
rg -n "class TimestampTransforms\|dispose()" libraries/scrcpy-decoder-webcodecs/src/video/utils/timestamp.ts -A 3Repository: yume-chan/ya-webadb
Length of output: 5097
Explicitly close #rawDecoder (VideoDecoder) in dispose().
The dispose() method relies on stream close callbacks to clean up #rawDecoder, but if dispose() is called while the pipeline is active (e.g., user closes the UI), the writable stream is not terminated and those callbacks never fire. This leaves the underlying VideoDecoder unclosed and hardware decoder resources potentially leaked. Call this.#rawDecoder.writable.getWriter().close() or abort the stream to ensure VideoDecoderStream's flush/cancel callbacks execute and properly close the decoder.
🤖 Prompt for AI Agents
In `@libraries/scrcpy-decoder-webcodecs/src/video/decoder.ts` around lines 238 -
247, The dispose() method must explicitly close the VideoDecoder stream to avoid
leaking hardware resources: ensure you force-close or abort this.#rawDecoder's
writable stream before disposing other helpers so the VideoDecoderStream
flush/cancel callbacks run; call this.#rawDecoder.writable.getWriter().close()
(or this.#rawDecoder.writable.getWriter().abort() on error) and await it (or
handle promise) so the underlying VideoDecoder is closed, then proceed to call
this.#size.dispose(), this.#pause.dispose(), this.#renderController.dispose(),
and this.#renderer.dispose?.().
Summary by CodeRabbit
New Features
Refactor
Other
✏️ Tip: You can customize this high-level summary in your review settings.